Allow model_version command line configuration of integration tests
#878
Conversation
There was a problem hiding this comment.
Hi @GernotMaier. I leave some comments here, but it was a bit hard to grasp some of the changes, as I am not very used to these configs. Let me know if my comments make sense. I will continue with the remaining PRs tomorrow
| @@ -22,6 +22,9 @@ jobs: | |||
| container: | |||
| image: ghcr.io/gammasim/simtools-dev:latest | |||
| options: --user 0 | |||
| strategy: | |||
| matrix: | |||
| type: ['2024-02-01', 'prod5'] | |||
There was a problem hiding this comment.
I was expecting this to run over prod6 as well. Is there a reason why it is not?
There was a problem hiding this comment.
Oh, 2024-02-01 is a prod6 prototype implementation. The model needs some more testing and some update, so better not to call it prod6.
There was a problem hiding this comment.
I tested now running the test for model_version="prod6" pytest --no-cov --model_version="prod6" "tests/integration_tests/test_applications_from_config.py::test_applications_from_config[simtools-print-array-elements_utm_to_ground_meta_in_yml]" and it works (the test also passes). I guess this is because we already have the model parameters for prod6 in the DB?
There was a problem hiding this comment.
Exactly. There is no difference between 2024-02-01 and prod6 (this is resolved in names.py)
tests/integration_tests/config/convert_model_parameter_from_simtel_num_gains.yml
Show resolved
Hide resolved
| if model_version is not None and "MODEL_VERSION" in config: | ||
| config.update({"MODEL_VERSION": model_version}) |
There was a problem hiding this comment.
I am not entirely sure these lines are doing what we expect. We prioritize here the parameter passed over the one in the config, but perhaps we gave the version in the config file because that is the one we expect it to work with, no?
There was a problem hiding this comment.
This replaces the model_version for all tools which have a model_version set via the config file.
This is exactly what we want to do and allow us to run the integration test using a command line parameter (e.g., --model_version=prod5 to run all integration tests with prod6.
There was a problem hiding this comment.
Ok, so that means all integration tests should work with both versions? For instance, we don't have integration tests that should work only with prod5 ?
There was a problem hiding this comment.
All tests should work with both versions (and they do). I do think we should aim that this is also true in the future.
The only difference we have to make if we check for output files. At this point, we only check for the version which is labeled in the configuration files (file naming is a bit complicated and adding this is probably not necessary)
|
@VictorBarbosaMartins - thanks a lot for the review. I've added back the missing warning. Let me know if it is fine now. |
There was a problem hiding this comment.
Thanks @GernotMaier. I have only two minor questions, so I am already approving
|
Thank you again. |
This PR adds an option to allow to run the integration tests for different model versions and adds a
--model_versionto the integration tests.Example:
Adds also a matrix job to the integration test CI to run over prod6 (default, labeled as 2024-02-01) and prod5.
Note that the additional tests (e.g., file content comparison) are disabled for the non-default version (which is given in conftest.py through the
model_versionfixture. Reason for that is that we otherwise would have to keep track and add comparison tables in test/resources for all model versions.Also solves a couple of issues to run simtools with the prod5 model version.