-
Notifications
You must be signed in to change notification settings - Fork 107
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
Do not retry when we get 'Job has finished' from qdel #7373
Conversation
assert "qdel executed" in Path(bin_path / "qdel_output").read_text(encoding="utf-8") | ||
if exit_code == QDEL_JOB_HAS_FINISHED: | ||
# the job has been already qdel-ed so no need to retry | ||
assert not os.path.exists(bin_path / "qdel_output") |
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.
Nitpick
assert not os.path.exists(bin_path / "qdel_output") | |
assert not (bin_path / "qdel_output").exists() |
# the job has been already qdel-ed so no need to retry | ||
assert not os.path.exists(bin_path / "qdel_output") | ||
else: | ||
assert "qdel executed" in Path(bin_path / "qdel_output").read_text( |
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.
Nitpick
assert "qdel executed" in Path(bin_path / "qdel_output").read_text( | |
assert "qdel executed" in (bin_path / "qdel_output").read_text( |
assert not os.path.exists(bin_path / "qdel_output") | ||
else: | ||
assert "qdel executed" in Path(bin_path / "qdel_output").read_text( | ||
encoding="utf-8" |
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.
Nitpick: Is encoding required? Is the default not "utf-8"
already?
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.
We should use the encoding. utf-8
is default on our computers yes, but lets be kind to the world.
See https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/unspecified-encoding.html
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 was enforced earlier when we had pylint in CI, now I see that our codebase has deterioated on this after the switch to ruff 😢
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.
From the PEP it sounds like it only is a problem on Windows. It's what we in the activism business called praxis in our fight against the Microsoft monopoly!
But, joking aside, it is reasonable to specify encoding="utf-8"
I guess. I always thought it was defined to be as such in Python 3, now that str
is unicode.
It's only Windows, and on Windows it looks like it's one of the Windows codepages, eg. cp1250
. Not even Windows uses this, preferring (the still bad) UTF-16 Unicode. Very confusing tbh. https://docs.python.org/3/library/locale.html#locale.getencoding
src/ert/scheduler/openpbs_driver.py
Outdated
QSUB_INVALID_CREDENTIAL, | ||
QSUB_PREMATURE_END_OF_MESSAGE, | ||
QSUB_CONNECTION_REFUSED, | ||
] | ||
|
||
QDEL_EXIT_CODES = [QDEL_REQUEST_INVALID, QDEL_JOB_HAS_FINISHED] | ||
QDEL_EXIT_CODES_TO_RETRY = [QDEL_REQUEST_INVALID] |
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.
Nitpick: the rest of the Scheduler code uses "returncode" (from asyncio.process.Process
's returncode
property), not "exit_code".
Also, perhaps this should be inlined with the qdel
code?
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.
I've removed the groups and inject them directly as individual return_codes 👍
src/ert/scheduler/openpbs_driver.py
Outdated
exit_codes_with_retries: List[int], | ||
exit_codes_without_retries: List[int], |
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.
Maybe less verbose, more to the point?
exit_codes_with_retries: List[int], | |
exit_codes_without_retries: List[int], | |
*, | |
retry_for: Iterable[int] = (), | |
accept: Iterable[int] = (), |
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.
Approving with nitpicks
5ab5f56
to
7776db7
Compare
Issue
Resolves #7369
Approach
Do not retry when qdel failed with
Job has finished
(Screenshot of new behavior in GUI if applicable)
When applicable