-
Notifications
You must be signed in to change notification settings - Fork 104
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
Refactor Lsf driver to use dataclasses #7915
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7915 +/- ##
==========================================
+ Coverage 85.82% 86.00% +0.17%
==========================================
Files 378 382 +4
Lines 23062 23601 +539
Branches 621 628 +7
==========================================
+ Hits 19794 20298 +504
- Misses 3189 3229 +40
+ Partials 79 74 -5 ☔ View full report in Codecov by Sentry. |
2237c92
to
7cd1029
Compare
This deviates from the way OpenPBS driver works. Should we keep them separate or also change OpenPBS to use dataclasses? |
I think it would be best to do them separately to keep the PRs smaller. |
It is not as obvious for OpenPBS that it should change to dataclass as it is for LSF, since it is actually using a json parser code from pydantic. So it might not be correct to port OpenPBS to dataclass, and then the question remains if LSF should be kept to Pydantic for consistency or not. The weight of Pydantic might not matter unless we have it measured it to be problematic. |
Yes, but is it not overkill to use Pydantic just to avoid implementing a |
It is a tradeoff we must make a decision on. |
I say we should merge this one, and I will get going on OpenPBS #7938 🚀 |
This commit refactors lsf driver from using pydantic classes to dataclasses, as pydantic was overkill for our usage.
d1f994d
to
b928c42
Compare
src/ert/scheduler/lsf_driver.py
Outdated
jobs: Mapping[str, AnyJob] | ||
def _create_job_class(job_dict: Mapping[str, str]) -> AnyJob: | ||
job_state = job_dict["job_state"] | ||
if job_state in get_type_hints(FinishedJobSuccess)["job_state"].__args__: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smells a little bit. Can we get away with just a dictionary from string job-state to the correct class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it looks way better but Mypy does not like that whatsoever.
src/ert/scheduler/lsf_driver.py:98: error: Argument 1 to "FinishedJobSuccess" has incompatible type "Literal['EXIT', 'DONE', 'PEND', 'RUN', 'ZOMBI', 'PDONE', 'SSUSP', 'USUSP', 'PSUSP', 'UNKWN']"; expected "Literal['DONE', 'PDONE']" [arg-type]
src/ert/scheduler/lsf_driver.py:98: error: Argument 1 to "FinishedJobFailure" has incompatible type "Literal['EXIT', 'DONE', 'PEND', 'RUN', 'ZOMBI', 'PDONE', 'SSUSP', 'USUSP', 'PSUSP', 'UNKWN']"; expected "Literal['EXIT', 'ZOMBI']" [arg-type]
src/ert/scheduler/lsf_driver.py:98: error: Argument 1 to "QueuedJob" has incompatible type "Literal['EXIT', 'DONE', 'PEND', 'RUN', 'ZOMBI', 'PDONE', 'SSUSP', 'USUSP', 'PSUSP', 'UNKWN']"; expected "Literal['PEND']" [arg-type]
src/ert/scheduler/lsf_driver.py:98: error: Argument 1 to "RunningJob" has incompatible type "Literal['EXIT', 'DONE', 'PEND', 'RUN', 'ZOMBI', 'PDONE', 'SSUSP', 'USUSP', 'PSUSP', 'UNKWN']"; expected "Literal['RUN', 'SSUSP', 'USUSP', 'PSUSP']" [arg-type]
src/ert/scheduler/lsf_driver.py:98: error: Argument 1 to "IgnoredJobstates" has incompatible type "Literal['EXIT', 'DONE', 'PEND', 'RUN', 'ZOMBI', 'PDONE', 'SSUSP', 'USUSP', 'PSUSP', 'UNKWN']"; expected "Literal['UNKWN']" [arg-type]
b928c42
to
563b810
Compare
a3780aa
to
04310e3
Compare
Issue
Resolves #7879
Approach
The commit in this PR refactors lsf driver from using pydantic classes to dataclasses, as pydantic was overkill for our usage.
(Screenshot of new behavior in GUI if applicable)
When applicable