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

New Dependencies etelemetry and neurdflib #3196

Closed
TheChymera opened this issue Mar 30, 2020 · 15 comments
Closed

New Dependencies etelemetry and neurdflib #3196

TheChymera opened this issue Mar 30, 2020 · 15 comments

Comments

@TheChymera
Copy link
Collaborator

I'm looking to update the nipype package for Gentoo Linux, I see that in the meantime two new dependencies have appeared, etelemetry and neurdflib. A cursory web search couldn't identify them unambiguously. Could you perhaps link me to the sources?

Also, regarding etelemetry, I came across this: https://github.com/mgxd/etelemetry-client . This makes it sound like it's a web interface, and makes me worry that depending on it could impair the capability of nipype to work offline --- i.e. in a sandboxed environment with restricted network access (particularly relevant for testing). If I am correct about this, is there any way to disable this feature other than patching the sources?

@mgxd
Copy link
Member

mgxd commented Mar 30, 2020

@TheChymera etelemetry is primarily used to fetch and compare version information (current vs latest available), and shouldn't fail offline (it will just complain no version information was found).

You can also disable its usage by setting an environmental variable NIPYPE_NO_ET=1

@satra
Copy link
Member

satra commented Mar 30, 2020

and for neurdflib you either have to package rdflib master or wait for it's next release (tentatively early april)

@TheChymera
Copy link
Collaborator Author

TheChymera commented Mar 30, 2020

thanks for your replies!

I am prepending NIPYPE_NO_ET=1 to the pytest command for the time being. Does this need to also be exported as a global variable for nipype not to fail during normal execution, or will it just pass that with a warning?

I am still getting two test failures when running the tests, one of them being related to fsl and the other one to etelemetry:

_____________________________________________________________ test_fslversion ______________________________________________________________

    @pytest.mark.skipif(no_fsl(), reason="fsl is not installed")
    def test_fslversion():
        ver = fsl.Info.version()
        ver = ver.split(".")
>       assert ver[0] in ["4", "5"]
E       AssertionError: assert '6' in ['4', '5']

../../../../work/nipype-1.4.2/nipype/interfaces/fsl/tests/test_base.py:17: AssertionError
________________________________________________________________ test_no_et ________________________________________________________________

tmp_path = PosixPath('/var/tmp/portage/sci-libs/nipype-1.4.2/temp/pytest-of-portage/pytest-0/test_no_et0')

    def test_no_et(tmp_path):
        from unittest.mock import patch
        from nipype.pipeline import engine as pe
        from nipype.interfaces import utility as niu
        from nipype.interfaces.base import BaseInterface

        # Pytest doesn't trigger this, so let's pretend it's there
        with patch.object(BaseInterface, "_etelemetry_version_data", {}):

            # Direct function call - environment not set
            f = niu.Function(function=_check_no_et)
            res = f.run()
>           assert res.outputs.out is True
E           assert False is True
E            +  where False = \nout = False\n.out
E            +    where \nout = False\n = <nipype.interfaces.base.support.InterfaceResult object at 0x7f0a6bdbf6d8>.outputs

The FSL one is fairly obvious --- but does nipype really not support FLS 6* ? I've been using fsl 6 with even older versions of nipype, and have not experienced any issues. Are you sure it's a good idea to make it fail when in most cases it would just work?

Regarding the etelemetry issue, I'm at a loss... any idea what's wrong?

@effigies
Copy link
Member

The FSL one is a very old test that used to be ver[0] == '4'. We should probably change to ver[0].isdigit().

@effigies
Copy link
Member

For the etelemetry test, looks like we're assuming that the environment variable is not set. I'll update both of these tests in #3195.

@mgxd
Copy link
Member

mgxd commented Mar 30, 2020

Does this need to also be exported as a global variable for nipype not to fail during normal execution, or will it just pass that with a warning?

If you want to disable nipype attempting to collect telemetry information, exporting the env var is the way to go. But again, etelemetry shouldn't cause a failure regardless of internet connection

@liamtimms
Copy link

liamtimms commented Jun 3, 2020

Hi guys just following up on this (and my issue a while back here: #3055), is it sufficient to just use the newest rdflib (5.0.0) and drop neurdflib completely with the newest nipype release (1.5.0)? It looks that way from requirements.txt but I just want to confirm before updating the nipype package for Arch Linux.

@satra
Copy link
Member

satra commented Jun 3, 2020

@liamtimms - confirmed.

@effigies effigies closed this as completed Jun 3, 2020
@TheChymera
Copy link
Collaborator Author

@mgxd @satra hi again, I have managed to run the test suite successfully with NIPYPE_NO_ET=1 pytest -vv. Sadly, however, when I use nipype, the import is still triggered. Any idea what else I can do other than patch the source?

Traceback (most recent call last):
  File "/home/chymera/.local/bin/SAMRI", line 11, in <module>
    load_entry_point('SAMRI', 'console_scripts', 'SAMRI')()
  File "/home/chymera/src/SAMRI/samri/cli.py", line 13, in main
    argh.dispatch_commands([diagnose, bru2bids, l1, generic, legacy])
  File "/usr/lib/python3.7/site-packages/argh/dispatching.py", line 328, in dispatch_commands
    dispatch(parser, *args, **kwargs)
  File "/usr/lib/python3.7/site-packages/argh/dispatching.py", line 174, in dispatch
    for line in lines:
  File "/usr/lib/python3.7/site-packages/argh/dispatching.py", line 277, in _execute_command
    for line in result:
  File "/usr/lib/python3.7/site-packages/argh/dispatching.py", line 260, in _call
    result = function(*positional, **keywords)
  File "/home/chymera/src/SAMRI/samri/pipelines/reposit.py", line 178, in bru2bids
    infosource = pe.Node(interface=util.IdentityInterface(fields=['subject_session'], mandatory_inputs=False), name="infosource")
  File "/usr/lib/python3.7/site-packages/nipype/interfaces/utility/base.py", line 61, in __init__
    super(IdentityInterface, self).__init__(**inputs)
  File "/usr/lib/python3.7/site-packages/nipype/interfaces/base/core.py", line 178, in __init__
    BaseInterface._etelemetry_version_data = check_latest_version()
  File "/usr/lib/python3.7/site-packages/nipype/__init__.py", line 83, in check_latest_version
    import etelemetry
ModuleNotFoundError: No module named 'etelemetry'

@effigies
Copy link
Member

It is a dependency, so I think it's reasonable to assume that it's importable. You can also set check_version to False before initiating any interfaces.

nipype.config.set("execution", "check_version", False)

@TheChymera
Copy link
Collaborator Author

@effigies thank you :) That's simple enough per-interface, but when designing lots of workflows it adds significant boilerplate. I think for the time being it might be better for me to patch that out of the code, since it's not really an essential functionality of nipype. Going forward, would you be interested in a PR setting up an environment variable to optionally disable (or enable ^^) version checking?

@effigies
Copy link
Member

NIPYPE_NO_ET=1 will disable it. Also, to be clear, you don't need to set the config for each interface. Once at import time will suffice.

@satra
Copy link
Member

satra commented Jul 20, 2020

it seems like two issues are being conflated here.

  1. etelemetry is a required dependency (therefore any packaging should require it as a dependency).
  2. whether etelemetry should be disabled or not in a workflow (this is up to each user of the library).

@TheChymera
Copy link
Collaborator Author

@effigies I tried to add NIPYPE_NO_ET=1 to my ~/.bashrc file, sourced it, and I still get the error.

@TheChymera
Copy link
Collaborator Author

ok, this is the sort of thing that happens when you don't remember your bash :D export NIPYPE_NO_ET=1 ofc worked. Thank you.

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

No branches or pull requests

5 participants