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

Adds MPI scheduling support #1673

Merged
merged 19 commits into from
Aug 12, 2020

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Aug 5, 2020

What do these changes do?

Allow the user to schedule services on MPI nodes. Please check proposed implementation in the liked issue.

Note: In development mode, the sidecar_mpi service will be spawned as an MPI node. This is done by setting the TARGET_MPI_NODE_CPU_COUNT environment variable to the number of CPUs of the host.

Related issue number

Closes #1672

How to test

Create a Study and add at least one copy of the following computational: sidecar, sidecar-gpu and sidecar-mpi.
Press play and check the logs of the sidecar_mpi swarm service.

Checklist

  • Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #1673 into master will decrease coverage by 0.3%.
The diff coverage is 36.1%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1673     +/-   ##
========================================
- Coverage    73.6%   73.3%   -0.4%     
========================================
  Files         278     279      +1     
  Lines       10923   11010     +87     
  Branches     1179    1189     +10     
========================================
+ Hits         8050    8078     +28     
- Misses       2530    2589     +59     
  Partials      343     343             
Flag Coverage Δ
#integrationtests 56.3% <25.0%> (-0.5%) ⬇️
#unittests 67.1% <35.2%> (-0.3%) ⬇️

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

Impacted Files Coverage Δ
.../director/src/simcore_service_director/producer.py 62.5% <0.0%> (-0.4%) ⬇️
...es/sidecar/src/simcore_service_sidecar/mpi_lock.py 27.2% <27.2%> (ø)
...src/simcore_service_sidecar/celery_configurator.py 49.3% <30.7%> (-6.1%) ⬇️
...rvices/sidecar/src/simcore_service_sidecar/core.py 77.5% <50.0%> (ø)
...vices/sidecar/src/simcore_service_sidecar/utils.py 89.8% <72.7%> (-3.4%) ⬇️
...ices/sidecar/src/simcore_service_sidecar/config.py 100.0% <100.0%> (ø)
...r/src/simcore_service_webserver/computation_api.py 65.6% <100.0%> (+0.2%) ⬆️
...es/sidecar/src/simcore_service_sidecar/rabbitmq.py 89.7% <0.0%> (-3.0%) ⬇️

@GitHK GitHK self-assigned this Aug 6, 2020
@GitHK GitHK added a:sidecar issue related with the sidecar worker service t:enhancement Improvement or request on an existing feature labels Aug 6, 2020
@GitHK GitHK added this to the Da Jia milestone Aug 6, 2020
@GitHK GitHK requested review from mguidon and sanderegg August 6, 2020 12:25
@GitHK GitHK marked this pull request as ready for review August 6, 2020 12:25
@GitHK GitHK changed the title WIP: Adds MPI scheduling support Adds MPI scheduling support Aug 6, 2020
async-timeout==3.0.1 # via aiohttp
attrs==19.3.0 # via aiohttp
aioredis==1.3.1 # via aioredlock
aioredlock==0.5.2 # via -r requirements/_base.in
Copy link
Member

Choose a reason for hiding this comment

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

hmm are you sure all these changes are needed? well not that I care too much but maybe for next time there is a way to only update the library you add doing something like:

make reqs update=NAMEOFLIBRARY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've actually added that library because I need it. Not happy about it, but the async version was the better one

Copy link
Member

Choose a reason for hiding this comment

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

I get that, I am a bit astonished that so many changes come in which is why I thought you updated more than just the aioredlock. whatever that is minor and if it's green it's green!

Copy link
Contributor Author

@GitHK GitHK Aug 11, 2020

Choose a reason for hiding this comment

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

My guess is that this project is not covered by depbot, so when I've updated with these 2 libraries, all others got updated.

Copy link
Member

@mguidon mguidon left a comment

Choose a reason for hiding this comment

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

Looks good. Sorry, I thought I already approved minus the wrong label in the sleeper.

@GitHK GitHK merged commit 43f358c into ITISFoundation:master Aug 12, 2020
@GitHK
Copy link
Contributor Author

GitHK commented Aug 17, 2020

adresses #1507

@sanderegg sanderegg mentioned this pull request Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:sidecar issue related with the sidecar worker service t:enhancement Improvement or request on an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPI celery queue on dalco cluster
4 participants