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

Require semantic version for model parameter version string #1103

Merged
merged 7 commits into from
Aug 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/CI-integrationtests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ jobs:
strategy:
fail-fast: false
matrix:
type: ['2024-02-01', 'prod5']
type: ['6.0.0', '5.0.0']

defaults:
run:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ for dir in "${model_directory}"*/; do
else
simtools-db-add-model-parameters-from-repository-to-db \
--model_version "${model-version}" \
--input_path "${dir}"/verified_model \
--input_path "${dir}" \
--db_name $SIMTOOLS_DB_SIMULATION_MODEL \
--type "model_parameters"
fi
Expand Down
2 changes: 1 addition & 1 deletion docs/source/user-guide/model_parameters.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ A typical model parameter file looks like:
"parameter": "num_gains",
"instrument": "LSTN-01",
"site": "North",
"version": "2024-02-01",
"version": "6.0.0",
"value": 2,
"unit": null,
"type": "int64",
Expand Down
6 changes: 3 additions & 3 deletions simtools/applications/db_get_array_layouts_from_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,20 @@

.. code-block:: console

simtools-db-get-array-layouts-from-db --site North --model_version "2024-02-01"
simtools-db-get-array-layouts-from-db --site North --model_version "6.0.0"

Retrieve telescope positions for array layout 'test_layout' from database.

.. code-block:: console

simtools-db-get-array-layouts-from-db --site North --model_version "2024-02-01"
simtools-db-get-array-layouts-from-db --site North --model_version "6.0.0"
--array_layout_name test_layout

Retrieve telescope positions from database (utm coordinate system) and write to an ecsv files

.. code-block:: console

simtools-db-get-array-layouts-from-db --site North --model_version "2024-02-01"
simtools-db-get-array-layouts-from-db --site North --model_version "6.0.0"
--array_element_list LSTN-01 LSTN-02 MSTN
--coordinate_system utm
--output_file telescope_positions-test_layout.ecsv
Expand Down
2 changes: 1 addition & 1 deletion simtools/applications/db_get_parameter_from_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
'File': True,
'Type': 'str',
'Value': 'mirror_CTA-N-LST1_v2019-03-31.dat',
'Version': '2020-06-28',
'Version': '5.0.0',
'_id': ObjectId('608834f257df2db2531b8e78'),
'entry_date': datetime.datetime(2021, 4, 27, 15, 59, 46, tzinfo=<bson.tz_util.FixedOffset \
object at 0x7f601dd51d80>)}
Expand Down
24 changes: 24 additions & 0 deletions simtools/data_model/validate_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ def _validate_data_dict(self):
if len(value_as_list) == 1:
self.data_dict["value"], self.data_dict["unit"] = value_as_list[0], unit_as_list[0]

self._check_version_string(self.data_dict.get("version"))

def _validate_data_table(self):
"""Validate tabulated data."""
try:
Expand Down Expand Up @@ -708,3 +710,25 @@ def _prepare_model_parameter(self):
if self.data_dict["unit"] is None
else gen.convert_string_to_list(self.data_dict["unit"])
)

def _check_version_string(self, version):
"""
Check that version string follows semantic versioning.

Parameters
----------
version: str
version string

Raises
------
ValueError
if version string does not follow semantic versioning

"""
if version is None:
return
semver_regex = r"^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not perfectly familiar with regex. Does this here allow also number.number.number-(additional expression)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - but see also #1115

if not re.match(semver_regex, version):
raise ValueError(f"Invalid version string '{version}'")
self._logger.debug(f"Valid version string '{version}'")
4 changes: 0 additions & 4 deletions simtools/db/db_from_repo_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ def update_model_parameters_from_repo(
parameter_collection,
model_version,
db_simulation_model_url,
db_simulation_model="verified_model",
):
"""
Update model parameters with values from a repository.
Expand Down Expand Up @@ -63,7 +62,6 @@ def update_model_parameters_from_repo(
db_simulation_model_url,
"model_versions",
model_version,
db_simulation_model,
array_element_name,
)
# use design array element model in case there is no model defined for this array element ID
Expand All @@ -75,7 +73,6 @@ def update_model_parameters_from_repo(
db_simulation_model_url,
"model_versions",
model_version,
db_simulation_model,
"OBS-" + site,
)
_design_model = None
Expand All @@ -95,7 +92,6 @@ def update_model_parameters_from_repo(
db_simulation_model_url,
"model_versions",
model_version,
db_simulation_model,
_design_model,
)
_tmp_par = gen.collect_data_from_file_or_dict(
Expand Down
2 changes: 1 addition & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ def pytest_addoption(parser):
@pytest.fixture()
def model_version():
"""Simulation model version used in tests."""
return "2024-02-01"
return "6.0.0"


@pytest.fixture()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ CTA_SIMPIPE:
SIMTEL_CFG_FILE: ./tests/resources/simtel_config_test_la_palma.cfg
SIMTEL_TELESCOPE_NAME: CT1
TELESCOPE: LSTN-01
MODEL_VERSION: "2024-02-01"
MODEL_VERSION: "6.0.0"
SCHEMA_DIRECTORY: ./tests/resources/
OUTPUT_PATH: simtools-tests
INTEGRATION_TESTS:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ CTA_SIMPIPE:
INPUT: tests/resources/array_element_position_ground.json
SITE: South
EXPORT: utm
MODEL_VERSION: "2024-02-01"
MODEL_VERSION: "6.0.0"
OUTPUT_PATH: simtools-tests
OUTPUT_FILE: test.json
LOG_LEVEL: DEBUG
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ CTA_SIMPIPE:
INPUT: tests/resources/array_element_position_utm.json
SITE: South
EXPORT: ground
MODEL_VERSION: "2024-02-01"
MODEL_VERSION: "6.0.0"
OUTPUT_PATH: simtools-tests
OUTPUT_FILE: test.json
LOG_LEVEL: DEBUG
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ CTA_SIMPIPE:
SIMTEL_CFG_FILE: ./tests/resources/simtel_config_test_la_palma.cfg
SIMTEL_TELESCOPE_NAME: CT1
TELESCOPE: LSTN-01
MODEL_VERSION: "2024-02-01"
MODEL_VERSION: "6.0.0"
SCHEMA: ./tests/resources/num_gains.schema.yml
OUTPUT_PATH: simtools-tests
OUTPUT_FILE: num_gains.json
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ CTA_SIMPIPE:
TEST_NAME: layout_list
CONFIGURATION:
SITE: North
MODEL_VERSION: "2024-02-01"
MODEL_VERSION: "6.0.0"
ARRAY_ELEMENT_LIST: ["LSTS", "MSTS", "SSTS"]
COORDINATE_SYSTEM: ground
OUTPUT_PATH: simtools-tests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ CTA_SIMPIPE:
TEST_NAME: layout_name
CONFIGURATION:
SITE: North
MODEL_VERSION: "2024-02-01"
MODEL_VERSION: "6.0.0"
ARRAY_LAYOUT_NAME: test_layout
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ CTA_SIMPIPE:
TEST_NAME: list_arrays
CONFIGURATION:
SITE: North
MODEL_VERSION: "2024-02-01"
MODEL_VERSION: "6.0.0"
LIST_AVAILABLE_LAYOUTS: True
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
CTA_SIMPIPE:
APPLICATION: simtools-validate-camera-efficiency
TEST_NAME: SSTS
MODEL_VERSION_USE_CURRENT: true
CONFIGURATION:
SITE: South
TELESCOPE: SSTS-design
Expand Down
2 changes: 1 addition & 1 deletion tests/resources/array_element_position_ground.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"parameter": "array_element_position_ground",
"instrument": "SSTS-09",
"site": "South",
"version": "2024-02-01",
"version": "6.0.0",
"value": "-4.79 -499.24 39.75",
"unit": "m",
"type": "float64",
Expand Down
2 changes: 1 addition & 1 deletion tests/resources/array_element_position_utm.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"parameter": "array_element_position_utm",
"instrument": "SSTS-09",
"site": "South",
"version": "2024-02-01",
"version": "6.0.0",
"value": "367321.0 7269466.0 2183.5",
"unit": "m",
"type": "float64",
Expand Down
2 changes: 1 addition & 1 deletion tests/resources/num_gains.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"parameter": "num_gains",
"instrument": "LSTN-01",
"site": "North",
"version": "2024-02-01",
"version": "6.0.0",
"value": 2,
"unit": null,
"type": "int64",
Expand Down
39 changes: 39 additions & 0 deletions tests/unit_tests/data_model/test_validate_data.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/python3

import logging
import re
import shutil
import sys
from importlib.resources import files
Expand Down Expand Up @@ -695,3 +696,41 @@ def test_prepare_model_parameter():
data_validator.data_dict["type"] = "int64"
data_validator._prepare_model_parameter()
assert isinstance(data_validator.data_dict["value"][0], int)


def test_check_version_string(caplog):
data_validator = validate_data.DataValidator()

valid_versions = [
"1.0.0",
"0.1.0",
"2.3.4",
"1.0.0-alpha",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to above question. So we really want to allow this kind of version structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how semantic versioning is defined, see https://semver.org/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok all good then.

"1.0.0-alpha.1",
"1.0.0-0.3.7",
"1.0.0-x.7.z.92",
"1.0.0-alpha+001",
"1.0.0+20130313144700",
"1.0.0-beta+exp.sha.5114f85",
]

for version in valid_versions:
with caplog.at_level("DEBUG"):
data_validator._check_version_string(version)
assert f"Valid version string '{version}'" in caplog.text
caplog.clear()

invalid_versions = [
"1.0",
"1.0.0.0",
"1.0.a",
"1.0.0-",
"1.0.0+",
"a.b.c",
]

for version in invalid_versions:
with pytest.raises(ValueError, match=f"Invalid version string '{re.escape(version)}'"):
data_validator._check_version_string(version)

assert data_validator._check_version_string(None) is None
21 changes: 8 additions & 13 deletions tests/unit_tests/db/test_db_from_repo_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ def test_update_parameters_from_repo(caplog, db_config, simulation_model_url):
parameters={},
site="North",
array_element_name="MSTN-01",
model_version="2024-02-01",
model_version="6.0.0",
parameter_collection="telescopes",
db_simulation_model_url=None,
db_simulation_model="verified_model",
)
== {}
)
Expand All @@ -33,10 +32,9 @@ def test_update_parameters_from_repo(caplog, db_config, simulation_model_url):
parameters=dict.fromkeys(_pars_telescope_model, None),
site="North",
array_element_name=_tel,
model_version="2024-02-01",
model_version="6.0.0",
parameter_collection="telescopes",
db_simulation_model_url=simulation_model_url,
db_simulation_model="verified_model",
)
assert len(_pars_mstn) > 0
assert "telescope_axis_height" in _pars_mstn
Expand All @@ -53,10 +51,9 @@ def test_update_parameters_from_repo(caplog, db_config, simulation_model_url):
parameters=dict.fromkeys(_pars_site_model, None),
site="South",
array_element_name=None,
model_version="2024-02-01",
model_version="6.0.0",
parameter_collection="site",
db_simulation_model_url=simulation_model_url,
db_simulation_model="verified_model",
)
assert len(_pars_south) > 0

Expand All @@ -66,10 +63,9 @@ def test_update_parameters_from_repo(caplog, db_config, simulation_model_url):
parameters=dict.fromkeys(_pars_telescope_model, None),
site="North",
array_element_name="MSTN-01",
model_version="2024-02-01",
model_version="6.0.0",
parameter_collection="not_a_collection",
db_simulation_model_url=simulation_model_url,
db_simulation_model="verified_model",
)

# Test with a parameter that is not in the repository (no error should be raised)
Expand All @@ -78,10 +74,9 @@ def test_update_parameters_from_repo(caplog, db_config, simulation_model_url):
parameters=dict.fromkeys(_pars_site_model, None),
site="South",
array_element_name=None,
model_version="2024-02-01",
model_version="6.0.0",
parameter_collection="site",
db_simulation_model_url=simulation_model_url,
db_simulation_model="verified_model",
)


Expand All @@ -92,7 +87,7 @@ def test_update_telescope_parameters_from_repo(db_config, simulation_model_url):
site="North",
array_element_name="MSTN-01",
parameter_collection="telescopes",
model_version="2024-02-01",
model_version="6.0.0",
db_simulation_model_url=simulation_model_url,
)
assert len(_pars) > 0
Expand All @@ -102,7 +97,7 @@ def test_update_telescope_parameters_from_repo(db_config, simulation_model_url):
site="North",
array_element_name="MSTN-design",
parameter_collection="telescopes",
model_version="2024-02-01",
model_version="6.0.0",
db_simulation_model_url=simulation_model_url,
)
assert len(_pars_design) > 0
Expand All @@ -119,7 +114,7 @@ def test_update_site_parameters_from_repo(db_config):
site="South",
array_element_name=None,
parameter_collection="site",
model_version="2024-02-01",
model_version="6.0.0",
db_simulation_model_url=db_config["db_simulation_model_url"],
)
assert len(_pars) > 0
Loading