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

Properly use kwargs for prefect's result target templating #41

Merged
merged 5 commits into from
Dec 16, 2021

Conversation

norlandrhagen
Copy link
Contributor

@norlandrhagen norlandrhagen commented Dec 15, 2021

Change Summary

Migrating this PR from esds-funnel.

While using funnel for caching, I ran into an unexpected error when trying to apply formatting for target names based on prefect parameters.

traceback: ```└── 16:41:35 | ERROR | Unexpected error: KeyError('gcm') Traceback (most recent call last): File "/Users/nrhagen/opt/anaconda3/envs/install/envs/cmip6-jupyter/lib/python3.9/site-packages/prefect/engine/runner.py", line 48, in inner new_state = method(self, state, *args, **kwargs) File "/Users/nrhagen/opt/anaconda3/envs/install/envs/cmip6-jupyter/lib/python3.9/site-packages/prefect/engine/task_runner.py", line 926, in get_task_run_state result = self.result.write(value, **formatting_kwargs) File "/Users/nrhagen/opt/anaconda3/envs/install/envs/cmip6-jupyter/lib/python3.9/site-packages/funnel/prefect/result.py", line 72, in write new = self.format(**{}) File "/Users/nrhagen/opt/anaconda3/envs/install/envs/cmip6-jupyter/lib/python3.9/site-packages/prefect/engine/result/base.py", line 133, in format new.location = new.location.format(**kwargs) KeyError: 'gcm' └── 16:41:35 | INFO | Task 'test_kwargs': Finished task run for task with final state: 'Failed' └── 16:41:35 | INFO | Flow run FAILED: some reference tasks failed. Flow run failed!

By editing line 72 of funnel/prefect/result.py from new = self.format(**{}) to new = self.format(**kwargs), the targeting templating seemed to succeed.

Credit goes to @tcchiao for finding a fix.

Related issue number

NCAR/esds-funnel#59

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

@andersy005
Copy link
Member

@norlandrhagen, should we update the return statement in exists()

https://github.com/NCAR/xpersist/blob/e9575692935495bd3ef85a6db059b8f2e66e5e4a/xpersist/prefect/result.py#L85-L97

to

 return location.format(**kwargs) in self.cache_store 

???

I saw this being used in one of Prefect's results object: https://github.com/PrefectHQ/prefect/blob/80efb2c4f509ba6ee324c6c5c9b214a745eae6a4/src/prefect/engine/results/local_result.py#L139

@andersy005 andersy005 added the enhancement New feature or request label Dec 16, 2021
@andersy005
Copy link
Member

Thank you, @norlandrhagen and @tcchiao! I'm going to merge this shortly

@andersy005 andersy005 merged commit 5e02e61 into ncar-xdev:main Dec 16, 2021
@andersy005 andersy005 changed the title minor edit to prefect/result.py l72 to update kwargs. Properly use kwargs for prefect's result target templating Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants