Skip to content

Fix process_results() not calling get_remote_files() #36

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

Merged
merged 4 commits into from
Feb 20, 2025
Merged

Conversation

araglu
Copy link
Collaborator

@araglu araglu commented Feb 7, 2025

After a job completed, this no longer downloaded remote files because of switching to the AsyncUitClient. This change uses async and await for get_remote_files(). It used to wander into Tethys code with process_results() and _process_results(), so I recreated that in a new function named async_process_results(). I also had to await the async_process_results() because it has the @database_sync_to_async decorator.

I am not that comfortable with async, so I definitely welcome any opinions around that. I am not confident that async_process_results() is a good name.

I don't use transfer_intermediate_files, so I have not been able to test that change.

CHW-680

After a job completed, this no longer downloaded remote files because of
switching to the AsyncUitClient. This change uses async and await for
get_remote_files(). It used to wander into Tethys code with
process_results() and _process_results(), so I recreated that in
async_process_results(). I also had to await the async_process_results()
because it has the @database_sync_to_async decorator.

I don't use transfer_intermediate_files, so I have not been able to test
that change.
@araglu araglu requested a review from sdc50 February 7, 2025 22:46
@sdc50
Copy link
Member

sdc50 commented Feb 11, 2025

@araglu I think that we should just name the function process_results since it is overriding the TethysJob.process_results function.

Possibly a future version of Tethys will support async job functions better and we won't have to override update_status or process_results but for now I think this is a good approach.

As for transfer_intermediate_files this looks good to me for now. I'd like to see if we can leverage this in the future to handle transferring results as they are completed.

@araglu
Copy link
Collaborator Author

araglu commented Feb 12, 2025

I renamed it to process_results() and removed the unused _safe_process_results(). It works well in my tests. Thanks!

@sdc50 sdc50 merged commit 27dabc6 into main Feb 20, 2025
2 checks passed
@araglu araglu deleted the results-async branch February 24, 2025 16:48
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