Skip to content

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

Draft
jbusecke wants to merge 4 commits into
mainfrom
fail-case-test-for-chained-function-injection
Draft

Traitlet injection fails with chained functions (e.g. beam resource hints)#102
jbusecke wants to merge 4 commits into
mainfrom
fail-case-test-for-chained-function-injection

Conversation

@jbusecke
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

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

@moradology
Copy link
Copy Markdown
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