Skip to content

[ENH] Migrating resource handler to Node level #1942

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 2 commits into from
May 1, 2017

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Apr 6, 2017

This PR:

  • Sets the default estimated_memory_gb to 0.25 (most of the nodes of a workflow will never reach 1GB)
  • Makes two new arguments available to creating Nodes (n_procs and mem_gb), that will set those two values in the inner interface (no need to set node.interface.num_threads anymore).
  • Fixes a small error formatting one Exception (we had __interface instead of _interface).

TODO list:

  • Remove estimated_memory_gb and num_threads from the BaseInterface
  • Find a way to synchronize the Node level num_threads and the inner interface inputs (maybe a new metadata threads=True to identify these inputs?)
  • Resource handling of MapNodes (check if these changes would affect those).

This PR:

- Sets the default `estimated_memory_gb` to 0 (most of the nodes of a workflow will never reach 1GB)
- Makes two new arguments available to creating Nodes (n_procs and mem_gb), that will set those two values in the inner interface (no need to call node.interface.num_threads anymore).
- Fixes a small error formating one Exception.

TODO list:

- Remove estimated_memory_gb and num_threads from the BaseInterface
- Find a way to synchronize the Node level num_threads and the inner interface inputs (maybe a new metadata threads=True to identify these inputs?)
- Resource handling of MapNodes (check if these changes would affect those).
@oesteban
Copy link
Contributor Author

oesteban commented Apr 6, 2017

@ccraddock may you have a look into these changes, and maybe give some feedback about the TODO list? Thanks!

@ccraddock
Copy link
Collaborator

I will have a look over the next few days. Really busy at the moment. But at first, I would recommend to make the default memory something > 0, such as .5 or .25

@codecov-io
Copy link

codecov-io commented Apr 6, 2017

Codecov Report

Merging #1942 into master will decrease coverage by 0.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1942      +/-   ##
==========================================
- Coverage   72.49%   72.47%   -0.02%     
==========================================
  Files        1063     1063              
  Lines       54159    54151       -8     
  Branches     7811     7812       +1     
==========================================
- Hits        39260    39246      -14     
- Misses      13680    13685       +5     
- Partials     1219     1220       +1
Flag Coverage Δ
#smoketests 72.47% <60%> (-0.02%) ⬇️
#unittests 70.01% <60%> (-0.02%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/plugins/multiproc.py 76.53% <0%> (ø) ⬆️
nipype/interfaces/base.py 84.2% <100%> (-0.46%) ⬇️
nipype/pipeline/engine/nodes.py 84.66% <66.66%> (-0.08%) ⬇️
nipype/interfaces/afni/base.py 58.94% <0%> (-0.85%) ⬇️
nipype/interfaces/utility/base.py 90.64% <0%> (-0.37%) ⬇️
nipype/interfaces/fsl/base.py 83.69% <0%> (-0.35%) ⬇️
nipype/interfaces/afni/utils.py 75.61% <0%> (-0.15%) ⬇️
nipype/interfaces/utility/tests/test_base.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3107a6d...b812031. Read the comment docs.

@oesteban
Copy link
Contributor Author

Up! cc/ @ccraddock

Copy link
Member

@satra satra left a comment

Choose a reason for hiding this comment

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

LGTM

@chrisgorgo chrisgorgo merged commit ae7af2d into nipy:master May 1, 2017
@oesteban oesteban deleted the enh/ResourceInitialization branch May 1, 2017 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants