Use latest patch version of the simulation model. #1768
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces automatic resolution of the latest patch version for simulation models to avoid hardcoding full version numbers in tests and configuration. The main purpose is to allow partial version specifications (e.g., "6.0") to be automatically resolved to the latest patch version (e.g., "6.0.2").
- Added version resolution functionality to automatically find the latest patch version for a given major.minor version
- Updated all test configuration files to use partial version numbers instead of full semantic versions
- Added version utility functions for semantic version manipulation and validation
Reviewed Changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simtools/version.py | Added new functions for version resolution and conversion to integers |
| src/simtools/db/db_handler.py | Integrated version resolution logic and clarified version variable naming |
| src/simtools/simulator.py | Moved semver_to_int function to version module |
| tests/unit_tests/test_version.py | Added comprehensive tests for new version functions |
| tests/conftest.py | Updated default model version fixture to use partial version |
| Multiple integration test configs | Changed model_version from full to partial versions (6.0.0 → 6.0) |
| Multiple unit test files | Updated tests to use partial versions and model_version fixture |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if pv.release and len(pv.release) >= 3: | ||
| return str(pv) |
There was a problem hiding this comment.
If you input resolve_version_to_latest_patch("5.2.1", versions) the output is 5.2.1 even if that doesn't exist in the available version because of this if statement. You need to also check if the pv exists in the available versions or not.
There was a problem hiding this comment.
I can see your point.
My thinking was that a function called resolve_version_to_latest_patch should simply return the full version string, if it is not necessary to search for the latest patch (e.g., in your example we do not need to search for 5.2.1.
Do you still think this should be changed?
There was a problem hiding this comment.
But if this function returns a version that's not in the available version list there will be an error later in the code when simtools tries to read data from the version, this might not be easily traced back to this function as there is no indication of an error or failure here
I think you can just raise an error here before the return if pv not in available_versions
There was a problem hiding this comment.
I get your point. This checked somewhere else and we get reasonable errors when we try to access a version which does not exist. The task of this function is to get the latest patch, I really don't want to mix tasks.
There was a problem hiding this comment.
Ok, if this is checked somewhere else then I'm fine to leave it as it is
|
|
Thanks for the review @EshitaJoshi |
Users and tests should be able to use the latest patch version of
model_version, without explicitly specify this. Especially tests should not have the full major.minor.patch version number hardwired.Introduce there the possibility to specify only major.minor for
model_versionand add functionality to retrieve the latest patch version. (meaning e.g.,6.0is currently resolved into6.0.1). Change the unit and integration tests to run with the model of the latest patch version (this unfortunately required a one-line change in most integration test configuration files).Try also to be a bit more clearer in db_handler on what is the released version of all simulation models (meaning the release version of the simulation models gitlab repo; now hopefully consistently called
db_simulation_model_version) and the actually production model version (calledmodel_versionthroughout simtools).Add version-string-related functions to the
simtools.versionmodule.This PR is required to get all of the flasher / laser / illuminator changes running.
Note #1767, which is probably related to this PR and needs to be fixed.