[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

Jump to bottom

Merged

chrisgorgo merged 2 commits into nipy:master from oesteban:enh/ResourceInitialization

May 1, 2017

Merged

[ENH] Migrating resource handler to Node level #1942

chrisgorgo merged 2 commits into nipy:master from oesteban:enh/ResourceInitialization

May 1, 2017

Conversation

@oesteban

Copy link

Contributor

@oesteban oesteban commented Apr 6, 2017 •

edited

Loading

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).

@oesteban


 [ENH] Migrating resource handler to Node level

92f6401

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

ccraddock commented Apr 6, 2017

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

@oesteban


 set memory minimum to 0.25 GB

b812031

@codecov-io

Copy link

codecov-io commented Apr 6, 2017 •

edited

Loading

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.

@mgxd mgxd added the needs-review label

Apr 26, 2017

@oesteban

Copy link

Contributor Author

oesteban commented Apr 29, 2017

Up! cc/ @ccraddock

satra

satra approved these changes

Apr 30, 2017

View reviewed changes

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

Labels

needs-review

6 participants

@oesteban @ccraddock @codecov-io @satra @chrisgorgo @mgxd

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ENH] Migrating resource handler to Node level #1942

[ENH] Migrating resource handler to Node level #1942

Uh oh!

Conversation

@oesteban oesteban commented Apr 6, 2017 •

edited

Loading

Uh oh!

Uh oh!

oesteban commented Apr 6, 2017

Uh oh!

ccraddock commented Apr 6, 2017

Uh oh!

codecov-io commented Apr 6, 2017 •

edited

Loading

Uh oh!

Codecov Report

Uh oh!

oesteban commented Apr 29, 2017

Uh oh!

@satra satra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[ENH] Migrating resource handler to Node level #1942

[ENH] Migrating resource handler to Node level #1942

Uh oh!

Conversation

@oesteban oesteban commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oesteban commented Apr 6, 2017

Uh oh!

ccraddock commented Apr 6, 2017

Uh oh!

codecov-io commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oesteban commented Apr 29, 2017

Uh oh!

@satra satra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

@oesteban oesteban commented Apr 6, 2017 •

edited

Loading

codecov-io commented Apr 6, 2017 •

edited

Loading