Do not retry when we get 'Job has finished' from qdel #7373
Conversation
xjules
added
release-notes:skip
| 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.
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.
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.
Nitpick: Is encoding required? Is the default not "utf-8" already?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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] = (), |
xjules
force-pushed
the
fix_qdel_retry
branch
3 times, most recently
from
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