Skip to content

[FIX] immunize shutil.rmtree to node non-existence for remove_node_di… #3135

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

Closed
wants to merge 52 commits into from

Conversation

dPys
Copy link

@dPys dPys commented Jan 3, 2020

Summary

  • Immunizes shutil.rmtree to node non-existence for remove_node_directories=True in the case that stop_on_first_crash=False. Without ignore_errors=True, workflows fail under these conditions.

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

…rectories=True in the case that stop_on_first_crash=False
@codecov
Copy link

codecov bot commented Jan 3, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@4e4b2a2). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3135   +/-   ##
=========================================
  Coverage          ?   67.59%           
=========================================
  Files             ?      299           
  Lines             ?    39480           
  Branches          ?     5220           
=========================================
  Hits              ?    26685           
  Misses            ?    12082           
  Partials          ?      713
Flag Coverage Δ
#smoketests 53.02% <0%> (?)
#unittests 64.83% <0%> (?)
Impacted Files Coverage Δ
nipype/pipeline/plugins/base.py 60% <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 4e4b2a2...75030b2. Read the comment docs.

oesteban and others added 28 commits January 6, 2020 10:42
The ``InterfaceHelpWriter`` class was practically a clone of
``ApiDocWriter``.

Both have been merged into one single module, having the Interface
helper inherit from the ApiDocWriter.
This PR follows up on nipy#3119 (merge that one first for a clean diff, or
diff against ``oesteban:maint/dedup-apigen-code``).

In practice, this PR fixes several broken points of our documentation
(e.g., the workflows list was empty and now it has been updated,
changelog not rendered, API of pure python code not rendered by the
Nipype API parser was missing, etc.).

CHANGES
-------

* Replaced the ``numpydoc`` sphinx extension with
``sphinxcontrib-napoleon``.
* Removed autosummary sphinx extension, required by numpydoc
* Cleared up ``docs/sphinxext/*``, as nothing is now used from there
* Use current sphinx-apidoc/autodoc/autosummary
* Removed the modref generation tooling, as it is not necessary anymore
  after re-enabling apidoc.
* Cut building warnings down to 321 - just those we incur because our
  API generator. This required some fixes of some docstrings.
  Beyond those corresponding to the Nipype API generator, only missing
  links remain as warnings (for sections in the navbar).
* Updated changelogs to be reStructuredText.
Building on top of nipy#3119 and nipy#3129, this PR makes a deep revision of the
documentation:

  * Added a new ``build_docs`` job to CircleCI to test how it renders.
  * Minimized external machinery (under ``/tools/``) when building the
    documentation:

      1. Some minimal modifications to sphinx extensions (apidoc,
         napoleon) allow the generation of special documentation
         for nipype interfaces, as it used to be before this PR
      2. A new sphinx extension (``nipype.sphinxext.apidoc``) takes
         care of parsing and rendering inputs and outputs. They now
         look like the parameters/arguments of functions when formatted
         with numpydoc.

  * Revised the description of many interfaces and the documentation of
    the main class and the input/output specs.
  * Revised the structure of the navbar, separating out
    User-Guide/Examples, Interfaces-Index, and Devs' documentation.
  * Minimized the number of WARNINGS at documentation build to 5 (4 of
    them coming out from the auto-generated SEM tools).
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: Chris Markiewicz <[email protected]>
In particular, after allowing printing the inheritance of interfaces,
links for ZZZCommandBase interfaces were broken, as originally filtered
out and not built. Now they are being built and we are back to 5
warnings.
oesteban and others added 11 commits January 6, 2020 10:49
Bringing CircleCI back to green after nipy#3124, nipy#3131, and nipy#3132.
By removing some space concatenation of strings, some of them were
r-strings and the concatenated one contained ``\n``, effectively
escaping the special return-carriage.

Instead of concatenating strings, the interface now accumulates the
lines in a list that is joined in the end.
Master is broken -- this PR relocates the pip install of
``niflow-nipype1-workflows`` so that it happens AFTER nipype
was installed.
@effigies
Copy link
Member

effigies commented Jan 6, 2020

Do you have a test?

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

A regression test would be nice. If infeasible, I'm comfortable merging, as ignore_errors has been around for all supported versions of Python.

This should be rebased onto maint/1.4.x, and it would probably be best if we wait on #3146 to rebase.

@dPys
Copy link
Author

dPys commented Jan 6, 2020

I could write a regression test for this, but it might be overkill? I can't really think of a case where ignore_errors=True would cause issues. As you noted, the flag has been around since Python 2. I do like the idea of waiting to rebase this into #3146 though, rather than rush the merge.

@effigies
Copy link
Member

effigies commented Jan 6, 2020

The regression test would be to make sure future changes don't restore this problem, not whether this fix causes new problems.

@effigies
Copy link
Member

effigies commented Jan 6, 2020

@dPys #3146 has been merged. Please rebase on maint/1.4.x and update the target:

Screen Shot 2020-01-06 at 12 58 41

@dPys
Copy link
Author

dPys commented Jan 6, 2020

The regression test would be to make sure future changes don't restore this problem, not whether this fix causes new problems.

Ahh, I see what you mean now.

@effigies
Copy link
Member

effigies commented Jan 6, 2020

@dPys This history looks a bit complicated. I would suggest the following to clean it up:

git fetch upstream
git checkout -b fix/rmtree_ignore_errors upstream/maint/1.4.x
git cherry-pick 1c98d4d
git push -u origin fix/rmtree_ignore_errors

Then create a new PR, closing this one.

@dPys dPys closed this Jan 6, 2020
@effigies
Copy link
Member

effigies commented Jan 7, 2020

Replaced by #3148.

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.

5 participants