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 support #369

Merged
merged 3 commits into from Oct 1, 2019
Merged

enh: add telemetry support #369

merged 3 commits into from Oct 1, 2019

Conversation

satra
Copy link
Member

@satra satra commented Sep 11, 2019

No description provided.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

I haven't tried yet, but great to see it coming!

if latest and 'version' in latest:
print("Your version: {0} Latest version: {1}".format(__version__,
latest["version"]))

Copy link
Member

Choose a reason for hiding this comment

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

not sure if this would be the right place to inform. I would like heudiconv --version output remain containing only the current version of the heudiconv

except RuntimeError as e:
print("Could not check for version updates: ", e)
else:
if latest and 'version' in latest:
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it compare __version__ to the latest somehow?

try:
latest = etelemetry.get_project("nipy/heudiconv")
except RuntimeError as e:
print("Could not check for version updates: ", e)
Copy link
Member

Choose a reason for hiding this comment

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

is there a >90% guarantee that etelemetry wouldn't crash for some other (IO/Syntax/whatever) reason? ;-) I would have caught all other exceptions (eg. except Exception as e) and issued a warning but kept going

Copy link
Member

Choose a reason for hiding this comment

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

use lgr.warning here?

@@ -14,6 +14,7 @@
'nipype>=1.0.0',
'pathlib',
'dcmstack>=0.7',
'etelemetry'
Copy link
Member

Choose a reason for hiding this comment

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

please add trailing , right away to minimize next diff ;)

print("Could not check for version updates: ", e)
else:
if latest and 'version' in latest:
print("Your version: {0} Latest version: {1}".format(__version__,
Copy link
Member

Choose a reason for hiding this comment

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

also I wonder if it should use lgr.info (or .warning) instead of a pure print

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.

Another thing to consider is CI usage skewing our metrics, I've created an issue at mgxd/etelemetry-client#1.

* upstream/master:
  Update heudiconv/info.py
  PIN: Track effigies/fix/heudiconv_issues
  PIN: Nipype master
@codecov
Copy link

codecov bot commented Sep 19, 2019

Codecov Report

Merging #369 into master will decrease coverage by 0.05%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
- Coverage   74.29%   74.24%   -0.06%     
==========================================
  Files          35       35              
  Lines        2708     2714       +6     
==========================================
+ Hits         2012     2015       +3     
- Misses        696      699       +3
Impacted Files Coverage Δ
heudiconv/info.py 100% <ø> (ø) ⬆️
heudiconv/cli/run.py 76.54% <57.14%> (-1.03%) ⬇️

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 fe0f2c8...6ade731. Read the comment docs.

@satra
Copy link
Member Author

satra commented Oct 1, 2019

@mgxd and @yarikoptic - could we merge this and cut a release?

@yarikoptic
Copy link
Member

@mgxd and @yarikoptic - could we merge this and cut a release?

Let's merge

Have you tried it "in a wild"? (would be nice to have some testing done before releasing)

@yarikoptic yarikoptic merged commit d31d19d into nipy:master Oct 1, 2019
@satra
Copy link
Member Author

satra commented Oct 1, 2019

a few other packages are sending signals. we need to make an interface to summarize the signals for the developer.

@yarikoptic yarikoptic added this to the 0.6 milestone Nov 22, 2019
yarikoptic added a commit that referenced this pull request Dec 16, 2019
This is largely a bug fix.  Metadata and order of `_key-value` fields in BIDS
could change from the result of converting using previous versions, thus minor
version boost.
14 people contributed to this release -- thanks
[everyone](https://github.com/nipy/heudiconv/graphs/contributors)!

 Enhancement

- Use [etelemetry](https://pypi.org/project/etelemetry) to inform about most
  recent available version of heudiconv. Please set `NO_ET` environment variable
  if you want to disable it ([#369][])
- BIDS:
  - `--bids` flag became an option. It can (optionally) accept `notop` value
    to avoid creation of top level files (`CHANGES`, `dataset_description.json`,
    etc) as a workaround during parallel execution to avoid race conditions etc.
    ([#344][])
  - Generate basic `.json` files with descriptions of the fields for
    `participants.tsv` and `_scans.tsv` files ([#376][])
  - Use `filelock` while writing top level files. Use
    `HEUDICONV_FILELOCK_TIMEOUT` environment to change the default timeout value
    ([#348][])
  - `_PDT2` was added as a suffix for multi-echo (really "multi-modal")
    sequences ([#345][])
- Calls to `dcm2niix` would include full output path to make it easier to
  discern in the logs what file it is working on ([#351][])
- With recent [datalad]() (>= 0.10), created DataLad dataset will use
  `--fake-dates` functionality of DataLad to not leak data conversion dates,
  which might be close to actual data acquisition/patient visit ([#352][])
- Support multi-echo EPI `_phase` data ([#373][] fixes [#368][])
- Log location of a bad .json file to ease troubleshooting ([#379][])
- Add basic pypi classifiers for the package ([#380][])

 Fixed
- Sorting `_scans.tsv` files lacking valid dates field should not cause a crash
  ([#337][])
- Multi-echo files detection based number of echos ([#339][])
- BIDS
  - Use `EchoTimes` from the associated multi-echo files if `EchoNumber` tag is
    missing ([#366][] fixes [#347][])
  - Tolerate empty ContentTime and/or ContentDate in DICOMs ([#372][]) and place
    "n/a" if value is missing ([#390][])
  - Do not crash and store original .json file is "JSON pretification" fails
    ([#342][])
- ReproIn heuristic
  - tollerate WIP prefix on Philips scanners ([#343][])
  - allow for use of `(...)` instead of `{...}` since `{}` are not allowed
    ([#343][])
  - Support pipolar fieldmaps by providing them with `_epi` not `_magnitude`.
    "Loose" BIDS `_key-value` pairs might come now after `_dir-` even if they
    came first before ([#358][] fixes [#357][])
- All heuristics saved under `.heudiconv/` under `heuristic.py` name, to avoid
  discrepancy during reconversion ([#354][] fixes [#353][])
- Do not crash (with TypeError) while trying to sort absent file list ([#360][])
- heudiconv requires nipype >= 1.0.0 ([#364][]) and blacklists `1.2.[12]` ([#375][])

* tag 'v0.6.0': (60 commits)
  Version boost to 0.6.0
  DOC: populate detailed changelog for 0.6.0 and tune up formatting in previous one
  Fix miscellaneous typos in ReproIn heuristic file.
  BF: fix check for the sbatch (SLURM) not being available
  ENH: make test-compare-two-versions take any two worktrees, and just show diff if results already known
  Update heudiconv/convert.py
  apply @mgxd 's suggestions, adding a warning and a timeout environment variable
  need str typecast
  Use empty string not None
  Empty acq_time results in empty cell not 'n/a'
  DOC: Clarify tarball session handling
  remove repetitive import statement
  respond to review - add explicit py2 check - change file saving strategy - use logger instead of print
  fix remaning py2 errors
  MNT: Add Python support metadata to package
  fix some python2/3 incompatibilities
  add return data (accidently removed return)
  make content unicode
  test that load_json provides filename if invalid
  explicitly name invalid json
  ...
yarikoptic added a commit that referenced this pull request Dec 16, 2019
[0.6.0] - 2019-12-16

This is largely a bug fix.  Metadata and order of `_key-value` fields in BIDS
could change from the result of converting using previous versions, thus minor
version boost.
14 people contributed to this release -- thanks
[everyone](https://github.com/nipy/heudiconv/graphs/contributors)!

Enhancement

- Use [etelemetry](https://pypi.org/project/etelemetry) to inform about most
  recent available version of heudiconv. Please set `NO_ET` environment variable
  if you want to disable it ([#369][])
- BIDS:
  - `--bids` flag became an option. It can (optionally) accept `notop` value
    to avoid creation of top level files (`CHANGES`, `dataset_description.json`,
    etc) as a workaround during parallel execution to avoid race conditions etc.
    ([#344][])
  - Generate basic `.json` files with descriptions of the fields for
    `participants.tsv` and `_scans.tsv` files ([#376][])
  - Use `filelock` while writing top level files. Use
    `HEUDICONV_FILELOCK_TIMEOUT` environment to change the default timeout value
    ([#348][])
  - `_PDT2` was added as a suffix for multi-echo (really "multi-modal")
    sequences ([#345][])
- Calls to `dcm2niix` would include full output path to make it easier to
  discern in the logs what file it is working on ([#351][])
- With recent [datalad]() (>= 0.10), created DataLad dataset will use
  `--fake-dates` functionality of DataLad to not leak data conversion dates,
  which might be close to actual data acquisition/patient visit ([#352][])
- Support multi-echo EPI `_phase` data ([#373][] fixes [#368][])
- Log location of a bad .json file to ease troubleshooting ([#379][])
- Add basic pypi classifiers for the package ([#380][])

Fixed

- Sorting `_scans.tsv` files lacking valid dates field should not cause a crash
  ([#337][])
- Multi-echo files detection based number of echos ([#339][])
- BIDS
  - Use `EchoTimes` from the associated multi-echo files if `EchoNumber` tag is
    missing ([#366][] fixes [#347][])
  - Tolerate empty ContentTime and/or ContentDate in DICOMs ([#372][]) and place
    "n/a" if value is missing ([#390][])
  - Do not crash and store original .json file is "JSON pretification" fails
    ([#342][])
- ReproIn heuristic
  - tolerate WIP prefix on Philips scanners ([#343][])
  - allow for use of `(...)` instead of `{...}` since `{}` are not allowed
    ([#343][])
  - Support pipolar fieldmaps by providing them with `_epi` not `_magnitude`.
    "Loose" BIDS `_key-value` pairs might come now after `_dir-` even if they
    came first before ([#358][] fixes [#357][])
- All heuristics saved under `.heudiconv/` under `heuristic.py` name, to avoid
  discrepancy during reconversion ([#354][] fixes [#353][])
- Do not crash (with TypeError) while trying to sort absent file list ([#360][])
- heudiconv requires nipype >= 1.0.0 ([#364][]) and blacklists `1.2.[12]` ([#375][])

* tag 'v0.6.0':
  Boost perspective release date in changelog to today
  ENH(TST): Fix version to older pytest to ease backward compatibility testing
  RF: use tmpdir not tmp_path fixture
  FIX: minor typo in CHANGELOG.md
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

3 participants