Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arbitrary injections #67

Closed
wants to merge 4 commits into from
Closed

Arbitrary injections #67

wants to merge 4 commits into from

Conversation

cisaacstern
Copy link
Member

WIP with @jbusecke to support arbitrary callable args injections.

Motivated by the desire to create "template recipes" for CMIP6 which can be called with arbitrary input parameters at recipe parse time.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #67 (4c2210c) into main (b42a5d1) will increase coverage by 0.00%.
The diff coverage is 86.36%.

@@           Coverage Diff           @@
##             main      #67   +/-   ##
=======================================
  Coverage   86.17%   86.18%           
=======================================
  Files          13       14    +1     
  Lines         405      427   +22     
=======================================
+ Hits          349      368   +19     
- Misses         56       59    +3     
Impacted Files Coverage Δ
pangeo_forge_runner/commands/bake.py 91.20% <60.00%> (-1.82%) ⬇️
pangeo_forge_runner/commands/_traits.py 94.11% <94.11%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cisaacstern
Copy link
Member Author

In @jbusecke and my limited testing, this featured worked for injecting a value into a "template recipe", here:

https://github.com/jbusecke/cmip6-leap-feedstock/blob/b0b72071935346a9630bff5f37e3b887e10c7481/feedstock/recipe.py#L12-L25

More thought can be given to how to best generalize this feature. The reserved function name we use currently is not as flexible as I imagine we'd want this to be before merging.

@cisaacstern
Copy link
Member Author

The reserved function name we use currently is not as flexible as I imagine we'd want this to be before merging.

@jbusecke, as of the last commit you can now call the inject_func in your recipe module whatever name you'd like. And as noted in the updated docstring, you will now pass the injection as a nested JSON-formatted dict, e.g.

--inject='{"get_some_value": {"value": 123}}'

where get_some_value is the function name, with argument value, defined in the recipe module. This also means you should be able to use arbitrary numbers of such functions, i.e.

--inject='{"get_some_value": {"value": 123}, "get_another_value": {"value": 456}}'

assuming get_some_value and get_another_value are both functions defined in your recipe module (with no limit on the number of such functions you can use).

@yuvipanda
Copy link
Collaborator

#86 is the plugin system for injections. However, I think that's a bit different from what is happening here. This is more of a 'traitlets pass through' - you want to have one recipe, and feed it different options from the commandline. Is that right?

Is there code that's using this already? One primary question for me is how this would affect the model of a recipe being reproducible. Right now, the params you pass on the commandline don't actually affect the work being done - only where it's sent to or cached or whatever. How would that change?

@jbusecke
Copy link
Contributor

I think those are valid concerns, and in fact @cisaacstern and I have gone a different route with the CMIP6 feedstock. I am not sure that this method is better in terms of reproducibility, but it works for now.

@cisaacstern
Copy link
Member Author

cisaacstern commented Aug 21, 2023

Yes, as noted by @jbusecke, we are not using this actively, and while I can't say for sure how much better the current approach is (not the entry points thing, I mean the link by Julius above), I think it is more intelligible than this, and am happy to see this be closed, as I was concerned that it was far too permissive (despite being the one who authored it 😆 ). Which is a long way of saying, thanks for taking a look, and I'll close now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants