Skip to content

Checking testing dependencies at runtime #2680

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
TheChymera opened this issue Aug 10, 2018 · 11 comments
Closed

Checking testing dependencies at runtime #2680

TheChymera opened this issue Aug 10, 2018 · 11 comments

Comments

@TheChymera
Copy link
Collaborator

TheChymera commented Aug 10, 2018

Summary

It appears =nipype-1.1.1 is checking at least one testing dependency at runtime and crashing all of my nodes if it is not present.
This really shouldn't be happening, especially not for a testing dep.
Shouldn't things just fail if+when the import statement fails?
I would like to sed the package for the 1.1.1 release to fix this behaviour, but I was unable to find the section of the code which does this.

Actual behavior

chymera@clusterhost ~/ni_data/ofM.dr/preprocessing $ nipypecli crash crashdump/crash-20180810-182150-chymera-get_s_scan.a177-134d9fcf-b564-4c40-a0c5-a0b398b38ac8.pklz
Traceback (most recent call last):
  File "/usr/lib/python-exec/python2.7/nipypecli", line 6, in <module>
    from pkg_resources import load_entry_point
  File "/usr/lib64/python2.7/site-packages/pkg_resources/__init__.py", line 3098, in <module>
    @_call_aside
  File "/usr/lib64/python2.7/site-packages/pkg_resources/__init__.py", line 3082, in _call_aside
    f(*args, **kwargs)
  File "/usr/lib64/python2.7/site-packages/pkg_resources/__init__.py", line 3111, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/usr/lib64/python2.7/site-packages/pkg_resources/__init__.py", line 573, in _build_master
    ws.require(__requires__)
  File "/usr/lib64/python2.7/site-packages/pkg_resources/__init__.py", line 891, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/lib64/python2.7/site-packages/pkg_resources/__init__.py", line 777, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'pytest-xdist' distribution was not found and is required by nipype

Platform details:

(nipype.get_info())"
{'commit_hash': '%h',
 'commit_source': 'archive substitution',
 'networkx_version': '2.1',
 'nibabel_version': '2.3.0',
 'nipype_version': '1.1.1',
 'numpy_version': '1.14.5',
 'pkg_path': '/usr/lib64/python2.7/site-packages/nipype',
 'scipy_version': '1.1.0',
 'sys_executable': '/usr/lib/python-exec/python2.7/python',
 'sys_platform': 'linux2',
 'sys_version': '2.7.15 (default, Aug  9 2018, 12:12:11) \n[GCC 6.4.0]',
 'traits_version': '4.6.0'}

Execution environment

  • My python environment outside container
@mgxd
Copy link
Member

mgxd commented Aug 13, 2018

@TheChymera It looks like the nipypecli crash command failed - could you install pytest-xdist and rerun it?

FWIW, 1.1.1 made pytest-xdist a mandatory dependency, and should have been included when updating to 1.1.1 (the motivation is mentioned here #2649 (comment)).

@TheChymera
Copy link
Collaborator Author

TheChymera commented Aug 14, 2018

@djarecka @oesteban @effigies I'd rather discuss this here than in the PR thread, but would you be interested in making (or instructing me on how to best make) the testing dependencies non-mandatory? Or generally eviscerating explicit dependency testing?

I don't think it's a good idea for things to fail unless they have to (makes the package less accessible), and as it stands a lot of non-testing functionalities fail just because some unrelated deps are not present.

I read the aforelinked conversation and I think this

(I mean we're asking for many outside standard library packages anyway)

is a rather problematic rationale. I'd rather put it the other way around and say that we already have heaps of deps and many potential failure points because of them, the least we could do is keep the absence of the non-critical ones non-critical.

@djarecka
Copy link
Collaborator

@TheChymera - well, I agree that you can argue with my rationale, but my main point was that in nipype we don't even try to limit ourselves to the Python Standard Library.

IMO, it's nice that users can test the library, but if others prefer to skip it, we can remove it. I believe that pytest is really pretty standard library, so we actually didn't have too many complains about it. The reason why you had problems was that we simply forgot to add one dependencies to conda.

@djarecka
Copy link
Collaborator

we can also consider limiting ourselves to pytest library only (i.e. removing xdist etc.)

@effigies
Copy link
Member

So specifically the pytest-xdist problem arises because we added -n auto to addopts in pytest.ini:

addopts = --doctest-modules -n auto

Unfortunately, there doesn't seem to be an easy way to use parallelism if pytest-xdist is installed and not pass failing arguments if not. On the whole, it seemed a pretty lightweight addition, but if we want to remove -n auto from the ini file, and add it to our CI test scripts instead, that's fine by me.

But anyway, my impression of this issue is that nipypecli checks dependencies, which seems to be more a feature of how scripts are installed than anything we've done...

@TheChymera
Copy link
Collaborator Author

but if others prefer to skip it, we can remove it

@djarecka yes, that's what I would also suggest.

But anyway, my impression of this issue is that nipypecli checks dependencies, which seems to be more a feature of how scripts are installed than anything we've done...

@effigies I think this is the actual issue here and it got conflated with some other issue regarding the way CI testing is performed. I'd be happy to help remove the explicit dependency check, but based on the text I am getting I can't figure out where exactly it gets generated. Can you help me out?

@djarecka
Copy link
Collaborator

@TheChymera - sorry, I mixed two issue, here indeed you shouldn't have the error when running your workflow. We should try to fix it.

Regarding suggestion of removing testing dependencies completely: let's ask other developers( @satra , @oesteban , @chrisfilo) if they prefer to remove all testing dependencies. I'm still voting for leaving pytest in the default installation.

@effigies
Copy link
Member

but based on the text I am getting I can't figure out where exactly it gets generated. Can you help me out?

It looks like it's a function of how scripts are installed.

nipype/setup.py

Lines 151 to 154 in 3dc5500

entry_points='''
[console_scripts]
nipypecli=nipype.scripts.cli:cli
'''

I haven't the slightest idea how to keep it from checking everything that's in install_requires.

@mgxd
Copy link
Member

mgxd commented Aug 15, 2018

indeed you shouldn't have the error when running your workflow.

is this the case? I wasn't able to replicate it in an environment without pytest-xdist
@TheChymera if so, can you post the stack trace?

@effigies
Copy link
Member

This is specifically a nipypecli issue. The console_scripts installs a stub executable that runs pkg_resources.load_entry_point, which appears to include a check for install_requires.

It's unclear how this got installed without installing all of the dependencies. @TheChymera How did you actually install nipype?

@effigies
Copy link
Member

Closed by #2850.

@effigies effigies added this to the 1.1.8 milestone Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants