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

Traitlet injection fails with chained functions (e.g. beam resource hints) #102

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jbusecke
Copy link
Contributor

@jbusecke jbusecke commented Sep 5, 2023

@cisaacstern and I just went on a bit of a deep dive to debug failures (example) over at cmip6-leap-feedstock, and discovered that the traitlet injection was failing for stages with resource hints like this:

...
StoreToZarr(....).with_resource_hints(min_ram=min_ram)
...

This PR adds a minimal failing example for the test_rewriter.py module.

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Patch coverage: 81.81% and project coverage change: -0.39% ⚠️

Comparison is base (e3c2dd1) 95.75% compared to head (4ea3f88) 95.37%.
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   95.75%   95.37%   -0.39%     
==========================================
  Files          13       13              
  Lines         424      432       +8     
==========================================
+ Hits          406      412       +6     
- Misses         18       20       +2     
Files Changed Coverage Δ
pangeo_forge_runner/recipe_rewriter.py 90.24% <81.81%> (-4.36%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cisaacstern
Copy link
Member

cisaacstern commented Sep 6, 2023

@jbusecke tests pass here (edit: except codecov...) so you could try merging this into your dev branch if you want to use resource hints while we await review. Or just not using them works to 😄 .

@jbusecke
Copy link
Contributor Author

jbusecke commented Sep 6, 2023

Or just not using them works to 😄 .

I think I am tending towards that for now, but might reconsider if big requests come in. Thanks for your work here!

@jbusecke
Copy link
Contributor Author

I think this feature might be useful for leap-stc/data-management#57.

@moradology
Copy link
Contributor

Looks to me like a potential symptom of this problem: #168

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