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

Basic rrq support #118

Merged
merged 18 commits into from
May 7, 2024
Merged

Basic rrq support #118

merged 18 commits into from
May 7, 2024

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented May 3, 2024

This PR adds the core of rrq support into hipercow; similar but different to workers in didehpc.

One thing that is not done is to run full hipercow tasks on a worker, I will follow up with OJ to find out if he does that. We'll also need some mechanism for deleting things as they will accumulate pretty badly.

Some other changes are flagged in comments. Docs will be added later, but that has the negative effect of most of the usage examples are some tests - only one of which actually does anything.

@richfitz richfitz marked this pull request as ready for review May 3, 2024 14:20
@richfitz richfitz requested a review from weshinsley May 3, 2024 14:20
Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5cfe124) to head (b4356db).
Report is 12 commits behind head on main.

❗ Current head b4356db differs from pull request most recent head b657f11. Consider uploading reports for the commit b657f11 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #118      +/-   ##
===========================================
+ Coverage   99.68%   100.00%   +0.31%     
===========================================
  Files          24        25       +1     
  Lines        2215      2293      +78     
===========================================
+ Hits         2208      2293      +85     
+ Misses          7         0       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@weshinsley weshinsley left a comment

Choose a reason for hiding this comment

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

OK - this looks good to me - had a good read, then updated the development bootstrap, and tested basic ops on the real cluster, which all went well. I wondered whether rrq::rrq_worker_stop() might be renamed to rrq::rrq_workers_stop(), but pffft. My hello world looked roughly like:

hipercow_init
hipercow_rrq_controller
hipercow_rrq_workers_submit
rrq::rrq_task_create_expr
rrq::rrq_task_result
rrq:rrq_worker_stop

which ends up being a very small number of very simple lines to get the idea.

I haven't tested hipercow_parallel(use_rrq) yet - I am struggling a bit to get my head around how that works with future/clusterApply to form a sensible example - (cf the comment in rrq.R about three levels of parallelism, and - diagram will definitely help there!! I think a documentation PR will be a nice time to play with that more and understand/explain it.

For now, this all looks good to me!

@weshinsley weshinsley merged commit 88201f0 into main May 7, 2024
14 checks passed
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