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 antares-study-version package to handle versions #79

Merged
merged 5 commits into from
Oct 10, 2024

Conversation

MartinBelthle
Copy link
Contributor

@MartinBelthle MartinBelthle commented Oct 8, 2024

⚠️ Breaking change: the sbatch command is now "8.8" instead of "880" when giving the study version.

@MartinBelthle MartinBelthle changed the title use antares-study-version package to handle versions use antares-study-version package to handle versions Oct 8, 2024
@@ -43,7 +43,7 @@ class StudyDTO:
# Simulation stage data
time_limit: t.Optional[int] = None
n_cpu: int = 1
antares_version: int = 0
antares_version: StudyVersion = StudyVersion.parse(0)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the parsing from tiny db will work ?
Also we need to take care that reading "old style" DTOs from tinydb still works, otherwise we will get exceptions when transitioning to this new version with old DTOs on disk

Copy link
Contributor Author

@MartinBelthle MartinBelthle Oct 8, 2024

Choose a reason for hiding this comment

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

Writing:
It is done inside method save_study like this:
new["antares_version"] = f"{new['antares_version']:2d}" line 93 of the file data_repo_tinydb.py

Reading:
It's done like this since last commit so it works for old versions too:

@classmethod
def from_dict(cls, doc: t.Mapping[str, t.Any]) -> "StudyDTO":
    """
    Create a Study DTO from a mapping.
    """
    attrs = dict(**doc)
    attrs.pop("name", None)  # calculated
    attrs["antares_version"] = StudyVersion.parse(attrs["antares_version"])
    return cls(**attrs)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I still added a smalle unit test to test this.

@MartinBelthle MartinBelthle requested a review from sylvlecl October 8, 2024 15:11
@MartinBelthle MartinBelthle changed the title use antares-study-version package to handle versions use antares-study-version package to handle versions Oct 8, 2024
Signed-off-by: Sylvain Leclerc <[email protected]>
Copy link
Member

@sylvlecl sylvlecl left a comment

Choose a reason for hiding this comment

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

Very minor comment, ok to merge after this

@@ -90,6 +90,7 @@ def save_study(self, study: StudyDTO):
self.db.update(new, tinydb.where(pk_name) == pk_value)
else:
logger.info(f"Inserting study '{pk_value}' in database: {new!r}")
new["antares_version"] = f"{new['antares_version']:2d}"
Copy link
Member

Choose a reason for hiding this comment

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

it's not very safe to do this because we change the study object:
if it's used after this method call, antares_version is a string and not a StudyVersion anymore.
It would be more safe to copy the dict for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay I see, modifying vars(study) alters the study object. I didn't think about that. I'll do a deepcopy of the dict.

@MartinBelthle MartinBelthle merged commit d613377 into main Oct 10, 2024
4 checks passed
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.

2 participants