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

enh: add telemetry to nipype #3027

Merged
merged 3 commits into from Sep 16, 2019
Merged

enh: add telemetry to nipype #3027

merged 3 commits into from Sep 16, 2019

Conversation

satra
Copy link
Member

@satra satra commented Sep 12, 2019

Summary

Adds the ability to check for the most recent released version.

List of changes proposed in this PR (pull-request)

  • add etelemetry as a dependency

Acknowledgment

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

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.

Seems fine. I looked at https://github.com/mgxd/etelemetry-client, and looks like it works:

$ curl https://rig.mit.edu/et/projects/nipy/nipype
{"version":"1.2.2"}

nipype/__init__.py Outdated Show resolved Hide resolved
nipype/__init__.py Outdated Show resolved Hide resolved
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #3027 into master will decrease coverage by 1.05%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3027      +/-   ##
==========================================
- Coverage   67.51%   66.46%   -1.06%     
==========================================
  Files         344      343       -1     
  Lines       44058    44050       -8     
  Branches     5552     5549       -3     
==========================================
- Hits        29746    29277     -469     
- Misses      13566    13985     +419     
- Partials      746      788      +42
Flag Coverage Δ
#smoketests 48.94% <80%> (-1.35%) ⬇️
#unittests 64% <ø> (-0.97%) ⬇️
Impacted Files Coverage Δ
nipype/utils/config.py 61.82% <ø> (-4.31%) ⬇️
nipype/info.py 94.36% <ø> (ø) ⬆️
nipype/__init__.py 59.57% <80%> (+0.11%) ⬆️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/testing/fixtures.py 77.33% <0%> (-21.34%) ⬇️
nipype/interfaces/freesurfer/base.py 61.01% <0%> (-19.5%) ⬇️
nipype/workflows/fmri/fsl/preprocess.py 72.67% <0%> (-13.39%) ⬇️
nipype/utils/filemanip.py 70.67% <0%> (-9.71%) ⬇️
nipype/workflows/smri/freesurfer/autorecon2.py 65.9% <0%> (-9.2%) ⬇️
nipype/workflows/smri/freesurfer/ba_maps.py 91.66% <0%> (-8.34%) ⬇️
... and 29 more

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 c69d4ad...7e7568e. Read the comment docs.

Copy link
Member

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

LGTM

@effigies effigies added this to the 1.2.3 milestone Sep 12, 2019
nipype/__init__.py Outdated Show resolved Hide resolved
@@ -68,6 +68,7 @@
parameterize_dirs = true
poll_sleep_duration = 2
xvfb_max_wait = 10
check_version = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a new entry in the documentation for this new option?

Copy link
Member Author

Choose a reason for hiding this comment

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

currently this is only at: https://miykael.github.io/nipype_tutorial/notebooks/basic_execution_configuration.html

i'll send a PR there as well.

Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
@satra
Copy link
Member Author

satra commented Sep 12, 2019

@mgxd - any thoughts on the py27 error?

@mgxd
Copy link
Member

mgxd commented Sep 12, 2019

It looks like it's not finding the versioneer.py with the package

My guess is adding a MANIFEST.in including these files should do the trick (wdyt @effigies)

EDIT: yep, I think that's it, pushing an updated et release now

@effigies
Copy link
Member

Is this all set?

@mgxd
Copy link
Member

mgxd commented Sep 16, 2019

@effigies let's wait to merge this until I cut a new release of etelemetry

@mgxd mgxd added the blocked label Sep 16, 2019
@effigies
Copy link
Member

@mgxd I'll leave it to you to merge when you're ready.


latest = {"version": 'Unknown'}
try:
latest = etelemetry.get_project("nipy/nipype")
Copy link
Member

Choose a reason for hiding this comment

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

@mgxd @satra It looks like etelemetry lets us send arbitrary kwargs. Should we add version information, so we can get a sense of which versions are still in use? e.g.,

latest = etelemetry.get_project("nipy/nipype", version=__version__)

Copy link
Member

Choose a reason for hiding this comment

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

that was intended to allow altering any requests specific arguments (such as timeout), but we could leverage params to do this.

@satra
Copy link
Member Author

satra commented Sep 20, 2019

or the .et file in a project repo.

the plan was to store the contents of the file (a json/ld dictionary) and send that back alongside version.

@effigies
Copy link
Member

effigies commented Sep 20, 2019

Happy to do it any way that works for you. IMO it would be nice to do this now, even if etelemetry doesn't support it yet, so that 1.2.3 can start reporting as soon as etelemetry gets upgraded.

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.

None yet

4 participants