SPD-410: stop functionality #16

lnauta · 2024-07-30T13:59:11Z

The functionality described in SPD-410 is added including tests and docstrings. Please review and let me know if theres anything missing.

The stop logic should be under user control. By putting it in a method its more easily accessible, instead of changing the template class in the example folder. The downside is that the EndlessViewIterator goes into a while loop whichs needs to be broken. As the logic in the run method is one level higher, its passed into the EVI if its being used. The while loop can get broken and stop the iterator after the stop condition is reached. The flow goes as follows: EVI.__next__ is called when the iterator is empty try: EVI.next calls the ViewIterator.next which calls TaskViewIterator.__next__ which calls TaskViewIterator.claim_task which calls _claim_task which fails and throws a StopIteration at EVI.next and the while-loop continues.

hailihu · 2024-09-03T12:10:32Z

Can you resolve merge conflicts, then I can start reviewing? Thanks.

lnauta · 2024-09-03T12:47:33Z

Ready to merge!

hailihu

Nice work! I still need to test it fully, but in the meantime you can have a look already at the suggestions/comments.

picas/actors.py

picas/executers.py

examples/local-example.py

picas/actors.py

Co-authored-by: Haili Hu <hailihu@gmail.com>

lnauta · 2024-09-04T14:15:20Z

Generally on the backwards compatibility:
I went for the design of
RunActor -> RunActorWithStop with the intention of eventually adding an abstract class as:
AbstractRunActor -> RunActor -> RunActorWithStop

We can change this into
AbstractRunActor (= current RunActor) -> RunActor (= current RunActorWithStop)
Where the abstraction will be done later, so its not really an Abstract.

Will that work?

hailihu · 2024-09-04T15:00:50Z

Concerning AbstractRunActor, I don't understand the need of making an abstract class.

hailihu · 2024-09-04T15:13:42Z

I tested user stories 2 (max execution time) and 3 (max no of tokens), but not user story 1 (max queue time) as that requires a long time. I assume this is sufficiently tested by test_signal_handling. Seems to work well!

@natalieda can you also review to see if I haven't missed anything?

lnauta · 2024-09-05T07:07:55Z

Concerning AbstractRunActor, I don't understand the need of making an abstract class.

The prepare and cleanup methods already show signs that we may want an ABC, but indeed it's not strictly necessary.
We can also call it BaseRunActor, or something similar.

hailihu · 2024-09-05T12:29:53Z

Concerning AbstractRunActor, I don't understand the need of making an abstract class.

The prepare and cleanup methods already show signs that we may want an ABC, but indeed it's not strictly necessary. We can also call it BaseRunActor, or something similar.

I see, in that case I agree with your previously mentioned option:
AbstractRunActor (= current RunActor) -> RunActor (= current RunActorWithStop)
to reduce the changes experienced by users.

tests/test_mock.py

examples/local-example.py

hailihu · 2024-09-16T09:01:31Z

After commit 610f99d, testing with python local-example.py fails. Requested changes below fix some bugs introduced since that commit.

examples/local-example.py

picas/executers.py

Co-authored-by: Haili Hu <hailihu@gmail.com>

lnauta added 12 commits July 19, 2024 15:51

Add RunActor base and RunActor stopping child

c15f026

Add Stop in Actor and example

ce1fd5c

Fix time limit stop fn in actors

f8bde84

Add handler when picas is killed

d1bd311

Rename reset value

2ca2246

Change executor and handler to gracefully kill token proc

86fbb7a

Change executor and handler to gracefully kill token proc

d85c94d

Add no resetting of token option

e12ccd8

Add unittests stopping code

26b613d

Add handler tests

e7f3fa0

Add docstrings actor tests

ec303b0

lnauta added the enhancement label Jul 30, 2024

lnauta requested a review from natalieda July 30, 2024 13:59

lnauta assigned natalieda Jul 30, 2024

lnauta added 7 commits July 30, 2024 16:00

Add logger to actors

04e6f8d

Fix import actors: subprocess

a1a5878

Fix reset bug on final finished token

46f2251

Fix stop fn failing on Nonetype

f2259e8

Fix reset token to use done instead of exit

782d5f9

Improve exit code default

d87cbbe

Update test log assertion

a252432

lnauta force-pushed the SPD-410 branch from 3154df3 to a252432 Compare August 2, 2024 11:42

Add token reset fix for when running is ended

6244b8c

lnauta mentioned this pull request Aug 2, 2024

SPD-409 Scrubbing added #17

Merged

lnauta added 3 commits August 2, 2024 17:04

Clean local example

1922e84

Fix style

b8eb4a7

Fix style

5284b8f

natalieda assigned hailihu and unassigned natalieda Aug 15, 2024

lnauta force-pushed the master branch from d33aa49 to ec8d177 Compare August 15, 2024 15:14

lnauta and others added 3 commits September 3, 2024 14:28

Merge branch 'master' into SPD-410

feb96d4

Fix test return value

9fff08c

Clean test

64dec5f

hailihu reviewed Sep 4, 2024

View reviewed changes

lnauta and others added 4 commits September 4, 2024 15:39

Update picas/actors.py

610f99d

Co-authored-by: Haili Hu <hailihu@gmail.com>

Add description to docstring actors

9237687

Fix execute proc fn

ebf1e8f

Fix execute proc fn

ee69025

Change actor names for backwards compatibility

5f426ad

hailihu approved these changes Sep 10, 2024

View reviewed changes

tests/test_mock.py Outdated Show resolved Hide resolved

examples/local-example.py Show resolved Hide resolved

Fix typo

b4f623e

hailihu self-requested a review September 16, 2024 08:56

hailihu requested changes Sep 16, 2024

View reviewed changes

examples/local-example.py Outdated Show resolved Hide resolved

examples/local-example.py Outdated Show resolved Hide resolved

picas/executers.py Outdated Show resolved Hide resolved

lnauta and others added 4 commits September 16, 2024 11:26

Update examples/local-example.py

4050ce0

Co-authored-by: Haili Hu <hailihu@gmail.com>

Update examples/local-example.py

7584f1f

Co-authored-by: Haili Hu <hailihu@gmail.com>

Update picas/executers.py

8c4cd63

Co-authored-by: Haili Hu <hailihu@gmail.com>

Revert executers test back

68f7898

hailihu approved these changes Sep 16, 2024

View reviewed changes

natalieda approved these changes Sep 17, 2024

View reviewed changes

natalieda merged commit f9577d8 into master Sep 17, 2024
4 checks passed

hailihu deleted the SPD-410 branch October 9, 2024 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPD-410: stop functionality #16

SPD-410: stop functionality #16

lnauta commented Jul 30, 2024

hailihu commented Sep 3, 2024 •

edited

Loading

lnauta commented Sep 3, 2024

hailihu left a comment

lnauta commented Sep 4, 2024 •

edited

Loading

hailihu commented Sep 4, 2024

hailihu commented Sep 4, 2024

lnauta commented Sep 5, 2024

hailihu commented Sep 5, 2024

hailihu commented Sep 16, 2024 •

edited

Loading

SPD-410: stop functionality #16

SPD-410: stop functionality #16

Conversation

lnauta commented Jul 30, 2024

hailihu commented Sep 3, 2024 • edited Loading

lnauta commented Sep 3, 2024

hailihu left a comment

Choose a reason for hiding this comment

lnauta commented Sep 4, 2024 • edited Loading

hailihu commented Sep 4, 2024

hailihu commented Sep 4, 2024

lnauta commented Sep 5, 2024

hailihu commented Sep 5, 2024

hailihu commented Sep 16, 2024 • edited Loading

hailihu commented Sep 3, 2024 •

edited

Loading

lnauta commented Sep 4, 2024 •

edited

Loading

hailihu commented Sep 16, 2024 •

edited

Loading