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

Wait for batch process API job to update status from PROCESSING to something else #536

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

mlubej
Copy link
Contributor

@mlubej mlubej commented Jul 31, 2024

A simple addition with more descriptions.

Another option would be to run the while loop as long as the status is PROCESSING, but perhaps the user would be confused why the job doesn't move forward as soon as the progress bars hit 100%. An additional reason is that the suggested implementation is perhaps more informative

@mlubej mlubej requested a review from zigaLuksic July 31, 2024 15:11
@mlubej mlubej self-assigned this Jul 31, 2024
@mlubej
Copy link
Contributor Author

mlubej commented Aug 1, 2024

@zigaLuksic in the monitor_batch_job we have the monitor_batch_analysis function which waits until the batch analysis is finished and then moves on. Then we put the responsibility of starting the job on the user, but if the user doesn't start the batch job, the loop will run forever.

Wouldn't it make more sense if the monitor_batch_job finishes early if the last user action was not START, and if it was, it waits for the PROCESSING status?

Asking because some test cases don't make much sense

@zigaLuksic
Copy link
Collaborator

@zigaLuksic in the monitor_batch_job we have the monitor_batch_analysis function which waits until the batch analysis is finished and then moves on. Then we put the responsibility of starting the job on the user, but if the user doesn't start the batch job, the loop will run forever.

Wouldn't it make more sense if the monitor_batch_job finishes early if the last user action was not START, and if it was, it waits for the PROCESSING status?

Asking because some test cases don't make much sense

Because monitor_batch_job is meant to monitor a started batch job. I'm not sure we want to think about all the possible cases of it being used wrong, because there are too many. Or do you think it's crystal clear?

@mlubej
Copy link
Contributor Author

mlubej commented Aug 1, 2024

Fixed the tests by additionally mocking the batch_client.get_request calls and updating the number of expected calls/loops

I'm not sure we want to think about all the possible cases of it being used wrong, because there are too many. Or do you think it's crystal clear?

It could very well be that I have a focus on a specific and this wouldn't hold in general, so I'm fine with leaving it like it is, I just wanted to point out that the tests simulate cases which don't seem to be realistic. I agree that it's hard to draw the line between the functionality and what is expected from the user. I guess that so far this has not been a problem, so let's leave it as it is.

@mlubej mlubej merged commit bc9abf1 into develop Aug 1, 2024
7 checks passed
@zigaLuksic zigaLuksic deleted the fix-bug-with-batch-job-status-update branch August 1, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants