-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for cancelled jobs. #23
base: main
Are you sure you want to change the base?
Changes from 181 commits
e22b4c2
c9912e5
df53ac8
4de9c10
a008530
6041591
05dab16
087d33a
acde2c8
a226f1a
425eb3f
3aebb1a
f5e51d2
7843d3e
0acc9ff
f587dd0
8c2d799
5a62309
c1ad0b3
ac5f3f9
88dbd27
889ca37
fb1efe4
e615b73
387d77f
a46aa97
93aaf93
c384731
c57184e
fb6e7ba
1c4b4ae
bf6323f
bda62a8
ffa64b8
9e58ae5
1e8eb6e
09c2dd0
346aee1
e073151
90ba635
acf7d7e
420b9d2
207af0e
b5f7c69
0e4e3e8
408ba47
392603a
f394a28
71012d2
bae01e7
eb2c713
d7f7760
f8a6f25
a2fe13d
862b311
e5a8f0f
c932edf
96900e3
0aa78d4
503e01c
1ed7c07
ef2f836
2a6aa08
faf8ef3
ca2b65c
36f89f5
74dc11d
2b39e7f
bbb55cf
7ea5a44
f35e97c
9b509b6
4cc2365
31b1266
e05bff2
1015971
3a819de
6c781e0
858e41c
68b66a2
afa3d61
4f21914
d71b734
2be9254
8f66ec9
b50b320
cf94ae7
627391d
68d44e6
65527c6
bff1ffc
d45418b
2dd3c33
36895e6
25156ed
1ef2518
8537f36
5124f60
7afb6c9
01b9ab6
0ff7d53
17b371a
30313ae
21a9181
a850441
0f85789
49811c9
92e293c
17b079f
6e1e25a
a576ebd
18444cc
e368fc8
b7e7e98
caa0b44
7c31978
c2dc930
30e409c
74cb01f
06a4d0f
e01d79c
447c2b4
e0981d9
d2343c5
305a183
c13e6ae
2e363e7
572df7d
eaf944c
374d11c
ea16318
3829159
24499a1
745226a
1e40385
243415d
65b5568
46ab4b1
b87482b
ffad748
6fc0577
6a948bd
ed38baa
8ef4cf5
8d96b30
e1fce94
f711d4d
939f14c
c32edd0
818c544
e9f30bd
f0f2c96
581a2da
846f7d7
26c9fa3
646207f
8c63d88
f50bc80
ede4947
0655a8f
f7030eb
15bc536
72442dd
d4c320c
34a6579
f5266b9
535deb6
b74c789
3b833f5
a1beeae
d97f2d0
c66165c
7301d0e
d46b194
3b78fa4
e134475
b83edb7
737268b
bcfe5c7
3d19ada
38d18c5
88c5dcb
e07ada3
6e985c3
db829fa
c4538fa
33ed863
03fd32d
b2e21b1
c3b4a07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,7 +109,7 @@ def _run_job(self, job: JobType) -> None: | |
|
||
self.state_store.update(job) | ||
|
||
if job.should_be_requeued(): | ||
if job.should_be_requeued: | ||
LOG.info("Requeuing job") | ||
self.queue.put(job) | ||
|
||
|
@@ -121,6 +121,9 @@ def run(self) -> None: | |
except KeyboardInterrupt: | ||
return | ||
|
||
if job.should_be_requeued: | ||
return | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this for? This would be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching that. I forgot the It's supposed to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This won't work if something gets cancelled in the state store (Netbox) if it doesn't get changed in the queue too, since this job object is obtained directly from the queue. The idea was to avoid an extra call to the state store every time a job was acquired. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Where would you recommend this check being made then? |
||
LOG.info("Job acquired") | ||
try: | ||
with DelayedKeyboardInterrupt(): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ class JobStatus(IntEnum): | |
COMPLETED = auto() | ||
FAILED = auto() | ||
ABORTED = auto() | ||
CANCELLED = auto() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ extras = test | |
|
||
[testenv:check] | ||
commands = | ||
ruff format --check ergate | ||
ruff format --diff ergate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot to undo this? :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not yet, anyway. ;) I was leaving it in until this issue was resolved: #23 (comment) Do you have any thoughts about that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to merge your PR with that since it does indeed appear unrelated. However I won't release until I get it fixed (possibly in a separate PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If my code subsequently is shown to have any issues once that is resolved, I'll plan to make a new PR to address them. |
||
ruff check ergate | ||
mypy ergate | ||
flake8 ergate | ||
|
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'd rather keep this as a method
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 reverted it, so that's fine. But, why? It's merely a computed value from another property which doesn't have external calls, so wouldn't it serve well as a property?