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

Delayed updates to config and metadata #198

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

gpetretto
Copy link
Contributor

Summary

Implementing delayed updates for config and metadata as discussed in #193

Added job name in the stored job document.

Fixing few issues in the update_config/metadata:

  • The function_filter was working only in the case of instantiated Jobs and not for @job decorated functions. Switched to checking the wrapped functions.
  • The logic for name_filter was wrong in case of the job name is None. In principle the name is never None, but fixed it anyway

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #198 (f006b92) into main (6a3f8a4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          19       19           
  Lines        1385     1401   +16     
  Branches      355      361    +6     
=======================================
+ Hits         1383     1399   +16     
  Misses          2        2           
Impacted Files Coverage Δ
src/jobflow/core/flow.py 100.00% <ø> (ø)
src/jobflow/core/job.py 100.00% <100.00%> (ø)

@utf
Copy link
Member

utf commented Sep 5, 2022

This looks great! Thanks very much.

My only comment is on the documentation. This is a complicated feature whose use won't be particularly obvious at first glance. Do you think you could include a minimal example in the docstring of for Job.update_config and Job.update_metadata and mention dynamic workflows and the response object?

@gpetretto
Copy link
Contributor Author

I added an explanation in the docstrings. Let me know if you think it needs some improvement.

@utf
Copy link
Member

utf commented Sep 7, 2022

Those examples are perfect. Thank you very much.

@utf utf merged commit 6c7b692 into materialsproject:main Sep 7, 2022
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.

2 participants