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: ExternalVersions.add - for extensions etc to add custom checkers #4441

Merged
merged 4 commits into from Apr 29, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Apr 23, 2020

Use case: datalad-neuroimaging started to fail tests. Preliminary
check seems to point toward newer dcm2niix.

It would be nice if extensions, where matters, could check for their
externals and possibly even register them within our "central"
ExternalVersions checker. This is what add effectively does. Also
for regular python modules, it would pretty much just allow to add
them to "INTERESTING" list - displayed by dumps.

I have positioned it against maint for now since it would allow extensions to use this feature sooner than later, and it is largely a minor enhancement which should not have negative side-effects (I hope ;)). But if there are objections, I am ok to change it to go to master.

Copy link
Collaborator

@kyleam kyleam left a comment

It would be nice if extensions, where matters, could check for their
externals and possibly even register them within our "central"
ExternalVersions checker.

Nice.

re: master vs maint: Not surprisingly I'd vote for this to go to master.

datalad/plugin/wtf.py Outdated Show resolved Hide resolved
datalad/support/external_versions.py Outdated Show resolved Hide resolved
datalad/support/external_versions.py Outdated Show resolved Hide resolved
datalad/support/external_versions.py Outdated Show resolved Hide resolved
datalad/support/external_versions.py Outdated Show resolved Hide resolved
datalad/support/external_versions.py Outdated Show resolved Hide resolved
datalad/support/external_versions.py Show resolved Hide resolved
datalad/support/tests/test_external_versions.py Outdated Show resolved Hide resolved
datalad/support/tests/test_external_versions.py Outdated Show resolved Hide resolved
@adswa

This comment has been minimized.

Use case: datalad-neuroimaging started to fail tests.  Preliminary
check seems to point toward newer dcm2niix.

It would be nice if extensions, where matters, could check for their
externals and possibly even register them within our "central"
ExternalVersions checker.  This is what `add` effectively does.  Also
for regular python modules, it would pretty much just allow to add
them to "INTERESTING" list - displayed by dumps.

Includes recommendations from Kyle's review.
@yarikoptic yarikoptic changed the base branch from maint to master Apr 27, 2020
@yarikoptic
Copy link
Member Author

yarikoptic commented Apr 27, 2020

Thanks @kyleam , I have pushed an update against master.

yarikoptic added 2 commits Apr 28, 2020
As discussed with @kyleam we have decided that it is not worth exposing
this whole notion of INTERESTING list, especially since only relevant ATM
to regular python packages.  So I have expanded the docstring to mention
the effect of calling "add" (regardless of "func") as to be included in
the dumps(query=True) output
This should centralize the chaining of CUSTOM and INTERESTING
kyleam
kyleam previously approved these changes Apr 28, 2020
Copy link
Collaborator

@kyleam kyleam left a comment

Thanks for the updates.

@kyleam kyleam dismissed their stale review Apr 28, 2020

The failing appveyor tests are pointing to a legitimate issue.

assert_not_in("numpy", ev.INTERESTING) # we do not have it by default yet
assert_not_in("numpy=", ev.dumps(query=True))
ev.add('numpy')
assert_in("numpy=", ev.dumps(query=True))
Copy link
Collaborator

@kyleam kyleam Apr 28, 2020

Choose a reason for hiding this comment

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

Fails on appveyor: https://ci.appveyor.com/project/mih/datalad/builds/32511871/job/ddbp8p80apy7i34r#L1038

I can trigger the same failure locally. I think the issue is that __getitem__ has an early return of None on an import error:

# doesn't pick it up from the _versions
if modname not in self._versions:
version = None # by default -- not present
if modname in self.CUSTOM:
try:
version = self.CUSTOM[modname]()
version = self._deduce_version(version)
except Exception as exc:
lgr.debug("Failed to deduce version of %s due to %s"
% (modname, exc_str(exc)))
return None
else:
if module is None:
if modname not in sys.modules:
try:
module = __import__(modname)
except ImportError:
lgr.debug("Module %s seems to be not present" % modname)
return None
except Exception as exc:
lgr.warning("Failed to import module %s due to %s",
modname, exc_str(exc))
return None
else:
module = sys.modules[modname]

So ._version.keys() will not have "numpy" present if numpy isn't installed in the environment. I think this test was assuming that the output would include "numpy=UNKNOWN" in this case. This feels like a logic bug in external_versions (exposed, not added, by this PR).

Copy link
Member Author

@yarikoptic yarikoptic Apr 29, 2020

Choose a reason for hiding this comment

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

THANK YOU for the analysis! I first thought that indeed code is wrong but then decided that code is correct as is: We do have a notion of "UNKNOWN" version but that is when the external is present but we do not know its version. If external is not present, then __getitem__ returns None, as to signal that it is really None, i.e. no any kind of version of the external is available.
I have pushed a fix to the test in d4dcc2c . Hopefully this would make this what should have been a trivial PR finally ready ;-)

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #4441 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4441   +/-   ##
=======================================
  Coverage   88.88%   88.89%           
=======================================
  Files         285      285           
  Lines       37844    37874   +30     
=======================================
+ Hits        33639    33669   +30     
  Misses       4205     4205           
Impacted Files Coverage Δ
datalad/plugin/wtf.py 82.27% <100.00%> (ø)
datalad/support/external_versions.py 96.07% <100.00%> (+0.45%) ⬆️
datalad/support/tests/test_external_versions.py 92.59% <100.00%> (+0.70%) ⬆️
datalad/dochelpers.py 87.40% <0.00%> (ø)

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 e7eedf0...d4dcc2c. Read the comment docs.

@kyleam kyleam merged commit f7e2d00 into datalad:master Apr 29, 2020
4 checks passed
@kyleam
Copy link
Collaborator

kyleam commented Apr 29, 2020

CrippledFS failure in last run looks unrelated: https://github.com/datalad/datalad/pull/4441/checks?check_run_id=628312224

@yarikoptic yarikoptic deleted the enh-externals branch May 21, 2020
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