Skip to content
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

SPD-409 Scrubbing added #17

Merged
merged 15 commits into from
Oct 11, 2024
Merged

SPD-409 Scrubbing added #17

merged 15 commits into from
Oct 11, 2024

Conversation

lnauta
Copy link
Member

@lnauta lnauta commented Aug 2, 2024

The scrubbing options have been added through examples where the Actor is customised.

@lnauta
Copy link
Member Author

lnauta commented Aug 2, 2024

Note that because this MR uses functionality from #16 that #16 needs to be merged into master first.

@natalieda natalieda requested a review from hailihu August 15, 2024 10:24
@natalieda natalieda assigned hailihu and unassigned natalieda Aug 15, 2024
@lnauta lnauta force-pushed the master branch 2 times, most recently from d33aa49 to ec8d177 Compare August 15, 2024 15:14
@hailihu
Copy link
Collaborator

hailihu commented Sep 23, 2024

Please merge with master and resolve conflicts.

@lnauta
Copy link
Member Author

lnauta commented Sep 23, 2024

Please merge with master and resolve conflicts.

Done!

Copy link
Collaborator

@hailihu hailihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to user story 2:
"As a Picas user, I want the tokens that exceed a specified expected execution time to be automatically resubmitted up to X times, so that I do not need to manually reset them every time."
However, the current implementation of the maximum execution time (either through max_time or stop_function) is different. Currently, the maximum execution time does not apply to the execution time of one token but all tokens together, because the Timer is initialized before the iterator loop. Also, a token is not forced to stop if max_time is exceeded, instead it is checked only after the token is already finished if max_time is exceeded.

If I understand the user story correctly, we need to use e.g. the stopit library: https://theautomatic.net/2021/11/27/how-to-stop-long-running-code-in-python/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the scrub_count functionality in a example files (scrub-example.py and scrub-timer-example.py) creates duplicate code (i.e. more difficult to maintain) and makes it more difficult for the users as they would need to adapt more code themselves.

I think it's better to have the scrub_count functionality in actors.py together with the stop functionality (RunActor.run). Also the scrub_count needs to be increased in case of the handler function is called (i.e. when Picas is killed).

@lnauta
Copy link
Member Author

lnauta commented Sep 26, 2024

I added an initial attempt for the stopit functionality, take a look if this is what youre looking for. I have not tested it yet.

I have also added the scrubbing to the handler. As for moving it to the actor class, can you make a suggestion on the implementation?

@hailihu
Copy link
Collaborator

hailihu commented Sep 27, 2024

I added an initial attempt for the stopit functionality, take a look if this is what youre looking for. I have not tested it yet.

I have also added the scrubbing to the handler. As for moving it to the actor class, can you make a suggestion on the implementation?

Find suggestion here: #23
You can merge if you approve into this branch, and I can continue testing and reviewing.

* Catch TimeoutExeption

* Move scrubbing to RunActor class

* Simplify max_time implementation

* Fix tests

* Check max_total_time for all iterators

* Added unit tests

---------

Co-authored-by: [email protected] <Haili Hu>
@hailihu
Copy link
Collaborator

hailihu commented Oct 8, 2024

While testing, I ran into this issue: #24. It is an already existing issue, in this case triggered by an Exception when a token exceeds the max_token_time. This results in "lost" tokens that are not visible in the Picas Views. Should we fix this before this PR is merged?

@hailihu
Copy link
Collaborator

hailihu commented Oct 9, 2024

While testing, I ran into this issue: #24. It is an already existing issue, in this case triggered by an Exception when a token exceeds the max_token_time. This results in "lost" tokens that are not visible in the Picas Views. Should we fix this before this PR is merged?

Suggestion to fix this error here: #25.

@lnauta
Copy link
Member Author

lnauta commented Oct 10, 2024

Suggestion to fix this error here: #25.

Has been merged into SPD-409

@lnauta lnauta merged commit 2e61dbb into master Oct 11, 2024
4 checks passed
@lnauta lnauta deleted the SPD-409 branch October 11, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants