-
Notifications
You must be signed in to change notification settings - Fork 536
enh: add telemetry to nipype #3027
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
Conversation
There was a problem hiding this 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"}Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>
@mgxd - any thoughts on the py27 error?
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
Is this all set?
@effigies let's wait to merge this until I cut a new release of etelemetry
@mgxd I'll leave it to you to merge when you're ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
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.
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.
Summary
Adds the ability to check for the most recent released version.
List of changes proposed in this PR (pull-request)
Acknowledgment