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

Polish celery/remote transformation strategy #240

Merged
merged 10 commits into from
Mar 7, 2023

Conversation

CasperWA
Copy link
Contributor

@CasperWA CasperWA commented Mar 2, 2023

Description:

Closes EMMC-ASBL/oteapi-services#246

Starting with a test for running the strategy, the related strategy models have been refined. An issue for using the pytest fixtures were found - the Celery documentation has been fixed in celery/celery#7857, but this has yet to be published, which resulted in lots of wasted time :(

Currently, the test is not appropriately testing the strategy, but rather injecting it's own "test" Celery App instead of the strategy's Celery App.
The test should be updated or another should be added to check the actual implementation of the strategy works as intended.

Through this work it has become apparent that an important step is missing: Creating a Celery Task that can actually be referenced in the get() method.
I think it makes most sense to register/create tasks in the initialize() step of the strategy. Another alternative is to have pre-defined tasks that can be referenced, however, this is slightly opaque for the user.

Note: One must run a local redis instance to test the celery/remote strategy.

Type of change:

  • Bug fix.
  • New feature.
  • Documentation update.

Checklist for the reviewer:

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?

@CasperWA CasperWA requested a review from quaat March 2, 2023 16:17
@CasperWA
Copy link
Contributor Author

CasperWA commented Mar 2, 2023

Great success concerning the new test and running it in CI: https://github.com/EMMC-ASBL/oteapi-core/actions/runs/4316062964/jobs/7531325447#step:8:89
Only thing is that we will have to split up the pytest CI job depending on OS as the Windows OS does not support using the services key.

@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: +0.69 🎉

Comparison is base (eaa965f) 81.20% compared to head (5bf9b69) 81.89%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     EMMC-ASBL/oteapi-core#240      +/-   ##
==========================================
+ Coverage   81.20%   81.89%   +0.69%     
==========================================
  Files          41       41              
  Lines        1128     1127       -1     
==========================================
+ Hits          916      923       +7     
+ Misses        212      204       -8     
Flag Coverage Δ
linux 81.81% <60.00%> (+0.60%) ⬆️
linux-extra_libs 81.81% <60.00%> (+0.60%) ⬆️
windows 81.31% <50.00%> (?)
windows-extra_libs 81.31% <50.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
oteapi/strategies/transformation/celery_remote.py 81.57% <60.00%> (+17.47%) ⬆️
oteapi/utils/paths.py 94.44% <0.00%> (+5.55%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@CasperWA CasperWA marked this pull request as ready for review March 6, 2023 11:48
Add test to check CeleryConfig can be instantiated with aliased and
non-aliased fields.
This is relevant when mixin aliases and non-aliases in the
`_use_session()` method.
@CasperWA CasperWA requested a review from quaat March 7, 2023 09:07
@CasperWA CasperWA merged commit 5ef872d into master Mar 7, 2023
@CasperWA CasperWA deleted the cwa/close-236-celery-strategy branch March 7, 2023 09:13
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.

Ensure celery-strategy is returning correct job-id
3 participants