-
Notifications
You must be signed in to change notification settings - Fork 2
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-410: stop functionality #16
Conversation
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.
Can you resolve merge conflicts, then I can start reviewing? Thanks. |
Ready to merge! |
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.
Nice work! I still need to test it fully, but in the meantime you can have a look already at the suggestions/comments.
Co-authored-by: Haili Hu <[email protected]>
Generally on the backwards compatibility: We can change this into Will that work? |
Concerning |
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 @natalieda can you also review to see if I haven't missed anything? |
The prepare and cleanup methods already show signs that we may want an ABC, but indeed it's not strictly necessary. |
I see, in that case I agree with your previously mentioned option: |
After commit 610f99d, testing with |
Co-authored-by: Haili Hu <[email protected]>
Co-authored-by: Haili Hu <[email protected]>
Co-authored-by: Haili Hu <[email protected]>
The functionality described in SPD-410 is added including tests and docstrings. Please review and let me know if theres anything missing.