-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactoring of build management #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things I noticed, looks nice!
Two points:
- We can postpone the Julia version discussion (and refactoring
Config
to a "better" approach) to a future issue. - I think we (= I) should come up with a reduced test set that calls the least amount of small models possible (since those should be tested by IESopt.jl) and just tests the relevant Python functionality, since running tests on all versions might otherwise take quite some time?
steps: | ||
- name: "set UV version" | ||
shell: bash | ||
run: echo "VERSION_UV=0.5.25" >> $GITHUB_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might that conflict with a fixed uv
version that we enforce somewhere else for reproducibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just here so that all CI steps use the same version of uv (and we can easily change this version in one location).
But I agree, its not optimal. Developers can use different uv versions locally, and I don't think we have any way to control that. It's probably not an big deal anyway, I just wanted the CI build to run with a well-defined uv version.
Co-authored-by: Stefan Strömer <8915976+sstroemer@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New commits + comments to address the feedback.
LGTM! Seems like we can merge this - or is there anything left open that would fit here? |
Various updates to the build system:
uv
#31 )