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

Use the qtoolkit and jobflow PyPI packages, add direct pydantic dep #31

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Oct 6, 2023

Closes #30

Related to #29

@ml-evs
Copy link
Member Author

ml-evs commented Oct 10, 2023

@gpetretto / @davidwaroquiers this would be helpful to merge before #29 is finished, then we can undo the changes after. (I also realise I do not have the power to request reviewers here... perhaps I could be added as a maintainer?)

@ml-evs ml-evs requested review from davidwaroquiers and gpetretto and removed request for davidwaroquiers October 10, 2023 14:45
@gpetretto
Copy link
Contributor

The current jobflow[strict] version pins pydantic to version 2.4.2. The latest jobflow version supporting pydantic <2 was 0.1.13. Maybe it would be better to also pin the jobflow requirement to that version? I am not sure if having pydantic<2 here could cause some conflicts.

Copy link
Member

@davidwaroquiers davidwaroquiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the comment on jobflow version wrt pydantic version. Not sure about it.

pyproject.toml Show resolved Hide resolved
@ml-evs
Copy link
Member Author

ml-evs commented Oct 10, 2023

The current jobflow[strict] version pins pydantic to version 2.4.2. The latest jobflow version supporting pydantic <2 was 0.1.13. Maybe it would be better to also pin the jobflow requirement to that version? I am not sure if having pydantic<2 here could cause some conflicts.

It could be pinned, but this seems like the lightest weight way of doing it -- all that #29 needs to do afterwards is lower pin pydantic~=2.0 and then jobflow>=0.1.14 will be used. Up to you if you'd rather pin jobflow too though!

@ml-evs
Copy link
Member Author

ml-evs commented Oct 10, 2023

I guess @gpetretto you also mean whether it is worth even adding pydantic as a dependency, instead just going along with jobflow? I think as we are using specific pydantic code here, then it should definitely appear as a dependency, even if the strict mode of jobflow is installing a specific version of it already. There were quite a few breaking pydantic changes even within the v1.x series...

@gpetretto
Copy link
Contributor

If pip resolves the correct version of jobflow to use it is fine as it is.

@ml-evs ml-evs merged commit 37124c3 into Matgenix:develop Oct 10, 2023
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.

Jobflow dependency
3 participants