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

Port sunsch to use Pydantic instead of configsuite #641

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

berland
Copy link
Collaborator

@berland berland commented Jan 3, 2024

No description provided.

@berland berland force-pushed the sunschpantic branch 4 times, most recently from bc0baf8 to 663fab4 Compare January 3, 2024 12:36
@codecov-commenter
Copy link

codecov-commenter commented Jan 3, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6c42a88) 86.13% compared to head (da21040) 86.13%.

❗ Current head da21040 differs from pull request most recent head 4c48229. Consider uploading reports for the commit 4c48229 to get more accurate results

Files Patch % Lines
src/subscript/sunsch/sunsch.py 98.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #641   +/-   ##
=======================================
  Coverage   86.13%   86.13%           
=======================================
  Files          49       49           
  Lines        7052     7047    -5     
=======================================
- Hits         6074     6070    -4     
+ Misses        978      977    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The v1 format has been purged from the code 3 years ago.
class SunschConfig(BaseModel):
files: Optional[List[FilePath]] = None
output: Optional[str] = "-"
startdate: Union[datetime.date, datetime.datetime]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would add: _ensure_start_refdate as a model validator, something like:

    @model_validator(mode="after")
    def ensure_start_date(self) -> Self:
        if not self.startdate:
            if self.starttime:
                self.startdate = self.starttime
            elif self.refdate:
                self.startdate = self.refdate
            else:
                self.startdate = datetime.date(1900, 1, 1)
        if not self.starttime:
            self.starttime = self.startdate
       if not self.refdate:
            self.refdate = self.startdate
        return self

could potentially also be a pre-validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, as a pre-validation. After-validation was not possible as it will invalidate based on the values given before this after_validator is run.

Copy link
Collaborator Author

@berland berland Jan 4, 2024

Choose a reason for hiding this comment

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

Tried also as after-validation by setting the date fields to be Optional, allowing such an after-validator:

    @model_validator(mode="after")
    def set_date_defaults(self) -> "SunschConfig":
        self.startdate = (
            self.startdate
            or self.starttime
            or self.refdate
            or datetime.date(1900, 1, 1)
        )
        self.starttime = self.starttime or datetime_from_date(self.startdate)
        self.refdate = self.refdate or self.startdate
        return self

This was even prettier but gave uglier changes to the rest of the code as all parts have to handle the Optional fields to make mypy happy. Will stick to the before-validator.

Copy link

Choose a reason for hiding this comment

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

I think in general before-validators are more practical for this kind of argument inference stuff

@berland berland requested a review from yngve-sk January 4, 2024 08:51

if args.verbose and config.snapshot.output != __MAGIC_STDOUT__:
config = SunschConfig(**merged_config)
Copy link

@yngve-sk yngve-sk Jan 4, 2024

Choose a reason for hiding this comment

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

Style: Could write something like this:

SunschConfig(**{
    "output": "-", 
    "startdate": datetime.date(1900, 1, 1),
    **yaml_config,
    **cli_config
})

instead of defining a default_config and doing .update calls etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, will try

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pydantic did not like merging these settings;
E TypeError: ModelMetaclass object got multiple values for keyword argument 'output'

Copy link

@yngve-sk yngve-sk left a comment

Choose a reason for hiding this comment

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

LGTM only some minor comments, as long the "using pre-validators for modifying the model's input dict" pattern works it is fine, would consider if overriding init and doing the input parsing there instead of a validator is a more clean expression of what is being done -- filling in "missing" arguments based on other arguments given. (defaulting startdate to starttime|refdate|datetime.date(1900,1,1), defaulting starttime to startdate, defaulting refdate to startdate)... would question why that logic is there in the first place but that is maybe outside the scope of this PR.

@berland
Copy link
Collaborator Author

berland commented Jan 4, 2024

Changed from using a data validator to to the transformation to overriding __init__.

@berland berland merged commit a37a641 into equinor:main Jan 4, 2024
5 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.

4 participants