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: run: Support globbing in uninstalled datasets #2796

Merged
merged 5 commits into from Sep 8, 2018

Conversation

@kyleam
Copy link
Member

@kyleam kyleam commented Sep 6, 2018

In order to resolve globbed paths within a subdataset, the subdataset
needs to be installed.  To do so, use the following procedure: install
the subdirectories returned by the longest *matched* pattern and then
glob again, stopping when no new paths are globbed.

[...]

This pull request fixes #2789.

kyleam added 5 commits Sep 5, 2018
I wanted to change test_run_inputs_outputs to
@with_testrepos("nested_submodule_annex", ...) to test globbing in
uninstalled subdatasets, but I've run into issues with git annex
considering subdatasets to be uninitialized.  To avoid going down that
rabbit hole at this moment, manually create the source repo rather
than using with_testrepos.  An upcoming commit will extend this with
nested subdatasets.
In preparation for making globbing work with uninstalled subdatasets,
we need to be able to re-glob when the file system has changed.
Chop off the tail of the pattern and stop at the first sub-pattern
that matches.  By doing this, we make it possible for the expand()
caller to match an uninstalled subdataset that contains a path,
install that subdataset, and repeat.

Note that if we weren't interested in supporting globs in the leading
path, we could avoid _get_sub_patterns and simply try to install the
leading path (if any) and then reglob.
In order to resolve globbed paths within a subdataset, the subdataset
needs to be installed.  To do so, use the following procedure: install
the subdirectories returned by the longest *matched* pattern and then
glob again, stopping when no new paths are globbed.

This relies on two features of the install call: (1) it will quietly
ignore subdirectories that aren't datasets and (2) it will install the
dataset when given a path to a directory within the dataset.

Punt on handling rerun_outputs.  At the moment the datalad.diff call
in rerun.py will fail if a subdataset isn't installed, and we need to
address that first.  (There's also the outstanding issue of the
subdataset state not being reset on rerun.)

The new tests pile on to the Windows incompatibility, but at this
point test_run.py needs a large-scale rewrite.

Fixes #2789.
@kyleam kyleam force-pushed the kyleam:bf-run-subds-glob branch from b6c6b59 to 2cf27db Sep 6, 2018
@codecov
Copy link

@codecov codecov bot commented Sep 6, 2018

Codecov Report

Merging #2796 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2796      +/-   ##
==========================================
+ Coverage    90.2%   90.23%   +0.03%     
==========================================
  Files         246      246              
  Lines       31643    31715      +72     
==========================================
+ Hits        28542    28617      +75     
+ Misses       3101     3098       -3
Impacted Files Coverage Δ
datalad/interface/run.py 100% <100%> (ø) ⬆️
datalad/interface/tests/test_run.py 99.82% <100%> (ø) ⬆️
datalad/cmdline/tests/test_main.py 95.65% <0%> (-1.41%) ⬇️
datalad/support/annexrepo.py 88.6% <0%> (ø) ⬆️
datalad/plugin/addurls.py 99.69% <0%> (ø) ⬆️
datalad/interface/tests/test_save.py 100% <0%> (ø) ⬆️
datalad/cmdline/helpers.py 70.96% <0%> (ø) ⬆️
datalad/distribution/tests/test_utils.py 100% <0%> (ø) ⬆️
datalad/downloaders/base.py 80.72% <0%> (ø) ⬆️
datalad/cmd.py 93.97% <0%> (ø) ⬆️
... and 8 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 5e59677...2cf27db. Read the comment docs.

Copy link
Member

@yarikoptic yarikoptic left a comment

time will show ... ;-) just a question about favorite OS of @mih

seen_magic = glob.has_magic(tail)
while head:
new_head, tail = op.split(head)
if seen_magic and not glob.has_magic(head):

This comment has been minimized.

@yarikoptic

yarikoptic Sep 7, 2018
Member

I quite dislike how "batteries included" often lack basic docstrings:

In [2]: glob.has_magic?
Signature: glob.has_magic(s)
Docstring: <no docstring>
File:      /usr/lib/python2.7/glob.py
Type:      function

This comment has been minimized.

@kyleam

kyleam Sep 7, 2018
Author Member

magic is very hard to define.

assert_false(Dataset(op.join(path, "s0")).is_installed())
ds.run("echo {inputs} >globbed-subds", inputs=["s0/s1_*/s2/*.dat"])
ok_file_has_content(op.join(ds.path, "globbed-subds"),
"s0/s1_0/s2/a.dat s0/s1_1/s2/c.dat",

This comment has been minimized.

@yarikoptic

yarikoptic Sep 7, 2018
Member

what will happen on Windows with all the paths here? just curious ;-)
I guess recorded run records would not be portable across platforms if we store them in "native" convention on Windows

This comment has been minimized.

@kyleam

kyleam Sep 7, 2018
Author Member

The tests have been incompatible with Windows from the beginning. I plan to move away from the use of echo, etc, as well as hard-coded "/". Then we can assess how much we need to change run/rerun to work on Windows, but I lean towards storing the paths in the commit message with "/" (like Git does) and then converting them when needed.

@kyleam
Copy link
Member Author

@kyleam kyleam commented Sep 7, 2018

time will show ... ;-)

I suppose you could say that about any non-trivial PR. Or do you have more specific concerns that make you say that?

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Sep 7, 2018

My primary concern is that quoting or escaping cmdline is tricky business. Have been burnt many times

@kyleam
Copy link
Member Author

@kyleam kyleam commented Sep 7, 2018

My primary concern is that quoting or escaping cmdline is tricky business.

I agree, though this PR doesn't really touch or change anything about quoting/escaping. It relies on has_magic, but glob.py guards its decision to glob with has_magic, so this PR at least doesn't add any new quoting/escaping issues, I think.

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Sep 8, 2018

Merged the other PR first and now this over in conflict

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Sep 8, 2018

@kyleam in such cases, if someone to help, should we use rebase or merge? ;)

@kyleam
Copy link
Member Author

@kyleam kyleam commented Sep 8, 2018

I'll merge it locally.

@kyleam kyleam merged commit 2cf27db into datalad:master Sep 8, 2018
10 checks passed
10 checks passed
WIP ready for review
Details
codecov/patch 100% of diff hit (target 90.2%)
Details
codecov/project 90.23% (+0.03%) compared to 5e59677
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
datalad-pr-dl-osx-64 DEV build done.
Details
datalad-pr-docker-dl-nd14_04 DEV build done.
Details
datalad-pr-docker-dl-nd16_04 DEV build done.
Details
datalad-pr-docker-dl-nd80 DEV build done.
Details
datalad-pr-docker-dl-nd90 DEV build done.
Details
kyleam added a commit that referenced this pull request Sep 8, 2018
Resolve trivial conflict between two added tests (keep both).
@kyleam kyleam deleted the kyleam:bf-run-subds-glob branch Sep 8, 2018
@yarikoptic yarikoptic added this to the Release 0.10.3 milestone Sep 12, 2018
yarikoptic added a commit that referenced this pull request Sep 13, 2018
0.10.3 (Sep 13, 2018) -- Almost-perfect

This is largely a bugfix release which addressed many (but not yet all)
issues of working with git-annex direct and version 6 modes, and operation
on Windows in general.  Among enhancements you will see the
support of public S3 buckets (even with periods in their names),
ability to configure new providers interactively, and improved `egrep`
search backend.

Although we do not require with this release, it is recommended to make
sure that you are using a recent `git-annex` since it also had a variety
of fixes and enhancements in the past months.

Fixes

- Parsing of combined short options has been broken since DataLad
  v0.10.0. ([#2710])
- The `datalad save` instructions shown by `datalad run` for a command
  with a non-zero exit were incorrectly formatted. ([#2692])
- Decompression of zip files (e.g., through `datalad
  add-archive-content`) failed on Python 3.  ([#2702])
- Windows:
  - colored log output was not being processed by colorama.  ([#2707])
  - more codepaths now try multiple times when removing a file to deal
    with latency and locking issues on Windows.  ([#2795])
- Internal git fetch calls have been updated to work around a
  GitPython `BadName` issue.  ([#2712]), ([#2794])
- The progess bar for annex file transferring was unable to handle an
  empty file.  ([#2717])
- `datalad add-readme` halted when no aggregated metadata was found
  rather than displaying a warning.  ([#2731])
- `datalad rerun` failed if `--onto` was specified and the history
  contained no run commits.  ([#2761])
- Processing of a command's results failed on a result record with a
  missing value (e.g., absent field or subfield in metadata).  Now the
  missing value is rendered as "N/A".  ([#2725]).
- A couple of documentation links in the "Delineation from related
  solutions" were misformatted.  ([#2773])
- With the latest git-annex, several known V6 failures are no longer
  an issue.  ([#2777])
- In direct mode, commit changes would often commit annexed content as
  regular Git files.  A new approach fixes this and resolves a good
  number of known failures.  ([#2770])
- The reporting of command results failed if the current working
  directory was removed (e.g., after an unsuccessful `install`). ([#2788])
- When installing into an existing empty directory, `datalad install`
  removed the directory after a failed clone.  ([#2788])
- `datalad run` incorrectly handled inputs and outputs for paths with
  spaces and other characters that require shell escaping.  ([#2798])
- Globbing inputs and outputs for `datalad run` didn't work correctly
  if a subdataset wasn't installed.  ([#2796])
- Minor (in)compatibility with git 2.19 - (no) trailing period
  in an error message now. ([#2815])

Enhancements and new features

- Anonymous access is now supported for S3 and other downloaders.  ([#2708])
- A new interface is available to ease setting up new providers.  ([#2708])
- Metadata: changes to egrep mode search  ([#2735])
  - Queries in egrep mode are now case-sensitive when the query
    contains any uppercase letters and are case-insensitive otherwise.
    The new mode egrepcs can be used to perform a case-sensitive query
    with all lower-case letters.
  - Search can now be limited to a specific key.
  - Multiple queries (list of expressions) are evaluated using AND to
    determine whether something is a hit.
  - A single multi-field query (e.g., `pa*:findme`) is a hit, when any
    matching field matches the query.
  - All matching key/value combinations across all (multi-field)
    queries are reported in the query_matched result field.
  - egrep mode now shows all hits rather than limiting the results to
    the top 20 hits.
- The documentation on how to format commands for `datalad run` has
  been improved.  ([#2703])
- The method for determining the current working directory on Windows
  has been improved.  ([#2707])
- `datalad --version` now simply shows the version without the
  license.  ([#2733])
- `datalad export-archive` learned to export under an existing
  directory via its `--filename` option.  ([#2723])
- `datalad export-to-figshare` now generates the zip archive in the
  root of the dataset unless `--filename` is specified.  ([#2723])
- After importing `datalad.api`, `help(datalad.api)` (or
  `datalad.api?` in IPython) now shows a summary of the available
  DataLad commands.  ([#2728])
- Support for using `datalad` from IPython has been improved.  ([#2722])
- `datalad wtf` now returns structured data and reports the version of
  each extension.  ([#2741])
- The internal handling of gitattributes information has been
  improved.  A user-visible consequence is that `datalad create
  --force` no longer duplicates existing attributes.  ([#2744])
- The "annex" metadata extractor can now be used even when no content
  is present.  ([#2724])
- The `add_url_to_file` method (called by commands like `datalad
  download-url` and `datalad add-archive-content`) learned how to
  display a progress bar.  ([#2738])

* tag '0.10.3': (214 commits)
  Changelog entry for 2.19 git compat fix
  DOC: slight tune ups to the ChangeLog
  ENH: link issue/pull #s in CHANGELOG, use tools/link_issues_CHANGELOG
  BF: remove trailing period while matching a mesage from git
  DOC: try_multiple_dec: Add description of `duration` parameter
  CLN: annexrepo: Fix grammar in a recently added comment
  TST: auto: Reformat comment from 900ee08
  DOC: _rmtree: Drop a word from summary line
  DOC: Info on IDs (fixes gh-2801)
  BF: diff -- when reraising - just raise, do not raise that instance of exception from new location
  BF(TST): precommit before removing submodule so there is no batched processes
  ENH+BF(TST): close established ssh sockets upon test finish
  BF(TST): One more "clost all log handlers in the test"
  CHANGELOG.md: Start adding entries for v0.10.3
  BF(TST): close cookies db in the teardown since atexit is later, so cannot assure no open files
  BF(TST): explicitly close created log handlers
  ENH(TST): @known_failure_windows to replace plain @skip_if_on_windows where there is hope
  BF(TST): do not swallow output while testing AutomagicIO to not cause some open files issue
  ENH(TST): Skip a test when cannot remove curent directory
  BF(TST): explicitly precommit a repo used under swallow_outputs
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants