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

BF/RF: automatically import needed interface submodules if Dataset.datasetmethod is not bound yet #3156

Merged
merged 9 commits into from Feb 12, 2019

Conversation

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Feb 10, 2019

This pull request fixes #3155

@yarikoptic yarikoptic added this to the Release 0.11.3 milestone Feb 10, 2019
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 10, 2019

Note: travis must be all green from now on. Someone yet to get hands dirty in Windows

Copy link
Contributor

@kyleam kyleam left a comment

The general approach looks good to me. From interactive testing, this seems to work well. My main comment is that I don't think test_datasetmethod_bound actually tests the changes in this PR. Even if I execute it directly with

coverage run $(which nosetests) -sv \
  datalad/distribution/tests/test_dataset.py:test_datasetmethod_bound \
  && coverage html

the lines of interest in Dataset.__getattr__ aren't covered because there are api imports needed by other tests. In order to see coverage with the above command, I have to make this change (which obviously breaks other tests in that module):

diff --git a/datalad/distribution/tests/test_dataset.py b/datalad/distribution/tests/test_dataset.py
index 503688e54..bd4664819 100644
--- a/datalad/distribution/tests/test_dataset.py
+++ b/datalad/distribution/tests/test_dataset.py
@@ -15,8 +15,6 @@
 
 from ..dataset import Dataset, EnsureDataset, resolve_path, require_dataset
 from datalad import cfg
-from datalad.api import create
-from datalad.api import get
 from datalad.utils import chpwd, getpwd, rmtree
 from datalad.utils import _path_
 from datalad.utils import get_dataset_root

@@ -100,6 +100,22 @@ def get_interface_groups(include_plugins=False):
return grps


def get_interface_name(intfspec, domain='python'):
Copy link
Contributor

@kyleam kyleam Feb 10, 2019

Doesn't get_api_name exist to handle "interface spec -> python api name" conversions?

@codecov
Copy link

@codecov codecov bot commented Feb 10, 2019

Codecov Report

Merging #3156 into master will increase coverage by 1.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3156      +/-   ##
==========================================
+ Coverage   89.47%   90.74%   +1.26%     
==========================================
  Files         248      249       +1     
  Lines       32703    32718      +15     
==========================================
+ Hits        29262    29689     +427     
+ Misses       3441     3029     -412
Impacted Files Coverage Δ
datalad/interface/tests/test_utils.py 100% <ø> (ø) ⬆️
datalad/interface/tests/test_run_procedure.py 100% <ø> (ø) ⬆️
datalad/distribution/uninstall.py 100% <ø> (ø) ⬆️
datalad/interface/annotate_paths.py 96.93% <ø> (-0.04%) ⬇️
datalad/interface/download_url.py 97.29% <ø> (-0.04%) ⬇️
datalad/interface/run.py 100% <ø> (ø) ⬆️
datalad/metadata/tests/test_base.py 99.31% <ø> (-0.01%) ⬇️
datalad/interface/tests/test_ls_webui.py 92.7% <ø> (-7.3%) ⬇️
datalad/distribution/subdatasets.py 94.69% <ø> (-0.04%) ⬇️
datalad/interface/rerun.py 96.2% <ø> (-0.04%) ⬇️
... and 45 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 ca3225e...ec0e2a0. Read the comment docs.

@codecov
Copy link

@codecov codecov bot commented Feb 10, 2019

Codecov Report

Merging #3156 into master will increase coverage by 1.32%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3156      +/-   ##
=========================================
+ Coverage   89.47%   90.8%   +1.32%     
=========================================
  Files         248     248              
  Lines       32703   33081     +378     
=========================================
+ Hits        29262   30038     +776     
+ Misses       3441    3043     -398
Impacted Files Coverage Δ
datalad/interface/tests/test_ls_webui.py 92.7% <ø> (-7.3%) ⬇️
datalad/interface/tests/test_utils.py 100% <ø> (ø) ⬆️
datalad/distribution/uninstall.py 100% <ø> (ø) ⬆️
datalad/metadata/tests/test_aggregation.py 99.06% <ø> (-0.01%) ⬇️
datalad/interface/run_procedure.py 89.24% <ø> (-0.07%) ⬇️
datalad/interface/tests/test_run_procedure.py 100% <ø> (ø) ⬆️
datalad/distribution/tests/test_install.py 99.4% <ø> (-0.01%) ⬇️
datalad/metadata/tests/test_base.py 99.31% <ø> (-0.01%) ⬇️
datalad/interface/base.py 90.32% <100%> (+0.28%) ⬆️
datalad/metadata/tests/test_search.py 93.47% <100%> (-0.05%) ⬇️
... and 32 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 ca3225e...1a75a3c. Read the comment docs.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 10, 2019

Than you Kyle!! Hard to answer per each on the phone, overall you are probably right on all points, please remove my function if there is already one to use - I must have missed it. Regarding ineffective test, I have made judgement based on the debug messages and they were there, so not sure how it wasn't in effect yet

To simplify testing without side-effects from import of datalad.api
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 10, 2019

re test not in effect -- I only can guess that I've removed those api imports but then while preparing the "clean commit" reverted those changes when found that they are needed... oh well -- thanks for catching! RFing now

yarikoptic added 2 commits Feb 10, 2019
Although may be when we decide to support plugins (see prev commit in the test)
then it would need to be tuned
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 11, 2019

As far as I care - it is ready to be merged. If anything, might just want some more manual testing

kyleam
kyleam approved these changes Feb 11, 2019
Copy link
Contributor

@kyleam kyleam left a comment

Thanks for the updates. Changes look good to me.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 11, 2019

actually now I think we should also add handling of plugins there for consistent behavior with .api imports (doing it now)

@kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 11, 2019

(doing it now)

OK. Just in case you didn't stumble upon load_interface:

diff --git a/datalad/distribution/dataset.py b/datalad/distribution/dataset.py
index 208f47bb4..8cae0d19c 100644
--- a/datalad/distribution/dataset.py
+++ b/datalad/distribution/dataset.py
@@ -179,16 +179,14 @@ def __getattr__(self, attr):
         # provided to @datasetmethod and what is defined in interfaces
         if not attr.startswith('_'):  # do not even consider those
             from datalad.interface.base import (
-                get_interface_groups, get_api_name
+                get_interface_groups, get_api_name, load_interface
             )
-            for _, _, interfaces in get_interface_groups():
+            for _, _, interfaces in get_interface_groups(include_plugins=True):
                 for intfspec in interfaces:
                     # lgr.log(5, "Considering interface %s", intfspec)
                     name = get_api_name(intfspec)
                     if attr == name:
-                        from importlib import import_module
-                        # turn the interface spec into an instance
-                        import_module(intfspec[0], package='datalad')
+                        load_interface(intfspec)
                         # Now it must be bound
                         meth = getattr(self, attr, None)
                         if meth:

yarikoptic added 2 commits Feb 11, 2019
… through plugins

It would avoid some wasted cycles, while also staying close imho to the original
behavior - I do not think we allow overload within interface specs, and order of plugins
is not guaranteed so we should not allow overload within plugins themselves
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 11, 2019

pushed, but I am not certain about 60b57d8 -- may be worth keeping it on the 6db6f6c to be safe (do go through all without early breaks) -- since that OPT unlikely to save much

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 11, 2019

uff -- thanks for all the findings - didn't run into load_interface indeed ... grr... will revert the last one and will go with exhaustive loop and load_interface

@kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 11, 2019

Failed run is due to a connectivity issue. Restarted.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 11, 2019

Ok, travis is green, all is good. Let's give @mih another day possibly to express his opinion and then merge.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 12, 2019

Let's hope for the best

@yarikoptic yarikoptic merged commit 81bfe17 into datalad:master Feb 12, 2019
2 of 3 checks passed
@yarikoptic yarikoptic deleted the bf-delayed-datasetmethod branch Feb 14, 2019
yarikoptic added a commit that referenced this issue Apr 6, 2019
 ## 0.11.3 (Feb 19, 2019) -- read-me-gently

 Just a few of important fixes and minor enhancements.

 ### Fixes

 - The logic for setting the maximum command line length now works
   around Python 3.4 returning an unreasonably high value for
   `SC_ARG_MAX` on Debian systems. ([#3165])

 - DataLad commands that are conceptually "read-only", such as
   `datalad ls -L`, can fail when the caller lacks write permissions
   because git-annex tries merging remote git-annex branches to update
   information about availability. DataLad now disables
   `annex.merge-annex-branches` in some common "read-only" scenarios to
   avoid these failures. ([#3164])

 ### Enhancements and new features

 - Accessing an "unbound" dataset method now automatically imports the
   necessary module rather than requiring an explicit import from the
   Python caller. For example, calling `Dataset.add` no longer needs to
   be preceded by `from datalad.distribution.add import Add` or an
   import of `datalad.api`. ([#3156])

 - Configuring the new variable `datalad.ssh.identityfile` instructs
   DataLad to pass a value to the `-i` option of `ssh`. ([#3149])
   ([#3168])

* tag '0.11.3': (35 commits)
  ENH: Finalized changelog entry for 0.11.3
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.3 entries
  CHANGELOG: Mention #3168
  ENH: sshconnector: Get identity file with datalad.cfg.get()
  [DATALAD RUNCMD] CHANGELOG: Linkify 0.11.3 entries
  CHANGELOG: Add entries for 0.11.3
  BF/RF(TST): skip test if actual sudo chown call fails
  BF(TMP): declare check_datasets_datalad_org failing on windows
  BF(TST): replace not relevant trailing .pull test with .repo_info
  BF(BK): for some reason an exception on repo_info invocation isn't raised while on travis
  TST: test_ro_operations  via sudo (when possible)
  BF: allow to disallow git-annex branch merges for repo_info and use that in ls
  BF(unicode): use assure_unicode while formatting an exception
  BF(PY3): workaround for python3.4 on debian returning obnoxious SC_ARG_MAX
  BF: revert change for repo_info about not merging remote git-annex
  BF: set annex.merge-annex-branches=false for invocations not requiring updated remote information
  ENH: ssh: Support configuring an identity file
  ENH: SSHConnection: Support custom identity files
  ENH: ssh: Add identity_file parameter to get_connection_hash()
  TST+BF: sshconnector: Fix stale socket path construction
  ...
yarikoptic added a commit that referenced this issue Apr 6, 2019
 ## 0.11.3 (Feb 19, 2019) -- read-me-gently

 Just a few of important fixes and minor enhancements.

 ### Fixes

 - The logic for setting the maximum command line length now works
   around Python 3.4 returning an unreasonably high value for
   `SC_ARG_MAX` on Debian systems. ([#3165])

 - DataLad commands that are conceptually "read-only", such as
   `datalad ls -L`, can fail when the caller lacks write permissions
   because git-annex tries merging remote git-annex branches to update
   information about availability. DataLad now disables
   `annex.merge-annex-branches` in some common "read-only" scenarios to
   avoid these failures. ([#3164])

 ### Enhancements and new features

 - Accessing an "unbound" dataset method now automatically imports the
   necessary module rather than requiring an explicit import from the
   Python caller. For example, calling `Dataset.add` no longer needs to
   be preceded by `from datalad.distribution.add import Add` or an
   import of `datalad.api`. ([#3156])

 - Configuring the new variable `datalad.ssh.identityfile` instructs
   DataLad to pass a value to the `-i` option of `ssh`. ([#3149])
   ([#3168])

* tag '0.11.3':
  BF(TST): skip if windows OR root; catch any exception
yarikoptic added a commit that referenced this issue Apr 6, 2019
 ## 0.11.3 (Feb 19, 2019) -- read-me-gently

 Just a few of important fixes and minor enhancements.

 ### Fixes

 - The logic for setting the maximum command line length now works
   around Python 3.4 returning an unreasonably high value for
   `SC_ARG_MAX` on Debian systems. ([#3165])

 - DataLad commands that are conceptually "read-only", such as
   `datalad ls -L`, can fail when the caller lacks write permissions
   because git-annex tries merging remote git-annex branches to update
   information about availability. DataLad now disables
   `annex.merge-annex-branches` in some common "read-only" scenarios to
   avoid these failures. ([#3164])

 ### Enhancements and new features

 - Accessing an "unbound" dataset method now automatically imports the
   necessary module rather than requiring an explicit import from the
   Python caller. For example, calling `Dataset.add` no longer needs to
   be preceded by `from datalad.distribution.add import Add` or an
   import of `datalad.api`. ([#3156])

 - Configuring the new variable `datalad.ssh.identityfile` instructs
   DataLad to pass a value to the `-i` option of `ssh`. ([#3149])
   ([#3168])

* tag '0.11.3':
  Boost version to 0.11.3
kyleam added a commit to kyleam/datalad that referenced this issue May 24, 2019
81bfe17 (Merge pull request datalad#3156 from
yarikoptic/bf-delayed-datasetmethod, 2019-02-12) made this
unnecessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants