materialsproject / jobflow

jobflow is a library for writing computational workflows.
https://materialsproject.github.io/jobflow
Other
90 stars 25 forks source link

allow different UID types #529

Closed jmmshn closed 6 months ago

jmmshn commented 6 months ago

Only allow UUIDs that can be sorted in time

Partially addresses: https://github.com/materialsproject/jobflow/issues/519

codecov[bot] commented 6 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (9bd99a8) 99.86% compared to head (923468b) 99.42%.

:exclamation: Current head 923468b differs from pull request most recent head 82ea1b5. Consider uploading reports for the commit 82ea1b5 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #529 +/- ## ========================================== - Coverage 99.86% 99.42% -0.45% ========================================== Files 20 21 +1 Lines 1514 1556 +42 Branches 417 422 +5 ========================================== + Hits 1512 1547 +35 - Misses 2 9 +7 ``` | [Files](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) | Coverage Δ | | |---|---|---| | [src/jobflow/core/flow.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvY29yZS9mbG93LnB5) | `100.00% <100.00%> (ø)` | | | [src/jobflow/core/job.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvY29yZS9qb2IucHk=) | `100.00% <100.00%> (ø)` | | | [src/jobflow/core/store.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvY29yZS9zdG9yZS5weQ==) | `100.00% <100.00%> (ø)` | | | [src/jobflow/settings.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvc2V0dGluZ3MucHk=) | `100.00% <100.00%> (ø)` | | | [src/jobflow/utils/\_\_init\_\_.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvdXRpbHMvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | | | [src/jobflow/utils/uuid.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvdXRpbHMvdXVpZC5weQ==) | `100.00% <100.00%> (ø)` | | | [src/jobflow/utils/uid.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/529?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvdXRpbHMvdWlkLnB5) | `81.57% <81.57%> (ø)` | |
gpetretto commented 6 months ago

Thanks for implementing the change and making uuid4 the default. It would be convenient for me (and maybe for others), not to have an abrupt change in the exposed functions as I am using the suuid function to generate a string uuid in a few places. It is not such a big deal, but would it be fine to keep the suuid function in the uuid module with a deprecation warning?

jmmshn commented 6 months ago

Yeah that makes sense.

jmmshn commented 6 months ago

BTW does anyone know what the s in suuid stands for?

utf commented 6 months ago

Thanks @jmmshn.

Is it possible to make ulid an optional dependency and only import it if the user has asked for it in their settings file. The code should also throw an appropriate error explaining which python package is needed to use ulid if it is not installed.

The s in suuid stands for string as we are encoding the UUIDs as strings not bytes.

utf commented 6 months ago

Thanks @jmmshn

jmmshn commented 6 months ago

BTW, can we get a release with this, then I can finish the atomate2 PR.

utf commented 6 months ago

Done (v0.1.17)