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

Return value revolution #1350

Closed
wants to merge 96 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mih
Member

mih commented Feb 28, 2017

Basic idea: commands yield report dictionaries. And wrapper act upon this information in whatever desired way.

Sits on top of #1348. Present tail of the procedure is in #1409

Change summary:

  • global command line switch to select output format
  • line-based 'json' output format (like git-annex)
  • 'simple' output format for "STATUS: PATH" lines, where PATH is relative whenever a reference dataset is available
  • supported commands return generators that are actually executed in main.py
  • centralized error detection that guarantees an exception if any error occured
  • BF: update did not fail on unavailable paths as argument
  • customize exception generation to be less dumb
  • centralize logging by shipping messages in the status dicts (actually: message template, variable, logger instance or func)
  • reimplement forced 'cmdline' rendering of API_ function with the new facilities to get rid of these shadow functions
  • convert actual return value nature on a by-call basis. Standard should be json dict sequence/generator.
  • provide return_value helper module with:
    - function to give a default dict
    - function to convert git-annex json record into our own structure
  • implement new constraint to test whether a dict has a key with a certain value
  • implement general command line options to filter results by type and by status (using new constraint above)
  • implement new general kwarg for all commands to accept a filter function for results (to fit a passed (compound) constraint) (name: filter_results)
  • implement new general kwarg for all commands to switch return values from generator to sequence (spec: return_type, {'generator', 'list', 'item-or-list'})
  • implement flexible transformation of return values from result dicts to other types (like Datasets)
  • implement general --on-failure switch (fixes #819)
  • new test helpers: assert_status, assert_message, assert_in_results, assert_not_in_results, assert_result_count, assert_fields_equal
  • #1380 support for result summaries

Commands converted:

  • create (possibly TODO: report addition to a superdataset)
  • install -- RF'ed to remove redundant code; split core functionality into a clone command (fully converted); high-level condition checking in install itself still needs conversion
  • get
  • add (complicated due to the old-school recursion in helper functions, needs substantial RF)
  • publish (seems straightforward)
  • uninstall (needs additional RF, PY2 version of Ben's class detector cannot deal with more than one class per module)
  • drop (needs additional RF, PY2 version of Ben's class detector cannot deal with more than one class per module)
  • remove (needs additional RF, PY2 version of Ben's class detector cannot deal with more than one class per module)
  • update (cmd itself is done, but needs to pass though results from get)
  • create-sibling (seems doable, unclear if RF needed to pass add-sibling results through)
  • create-sibling-github (doable)
  • add-sibling (maybe better wait till after general RF; convoluted already)
  • rewrite-urls (does not seem to be used anywhere -> low priority; but otherwise simple)
  • unlock
  • save
  • export (seems doable, but also rather useless)
  • search (will not be attempted, waiting for metadata RF)
  • aggregate-metadata (will not be attempted, waiting for metadata RF)
  • test (not needed)
  • crawl, crawl-init (no idea if it is feasible to attempt this, better be done by Yarik, also crawl-init about to be gone #1155)
  • ls (will not be attempted, does not fit paradigm as of now, but could become relevant after RF)
  • clean (looks straightforward)
  • add-archive-content
  • download-url (seems straightforward, custom stop_on_failure needs to be RF'ed away)
  • create-test-dataset
  • sshrun (out of scope, needs to stay as it is)

bpoldrack added some commits Feb 28, 2017

RF: Use better_wraps with @datasetmethod
RF: Fix datasetmethods' docstrings at the same time we do it for the datalad.api functions
@codecov-io

This comment has been minimized.

codecov-io commented Feb 28, 2017

Codecov Report

Merging #1350 into master will decrease coverage by 55.32%.
The diff coverage is 51.41%.

@@            Coverage Diff             @@
##           master   #1350       +/-   ##
==========================================
- Coverage   89.42%   34.1%   -55.33%     
==========================================
  Files         236     237        +1     
  Lines       24837   24856       +19     
==========================================
- Hits        22211    8476    -13735     
- Misses       2626   16380    +13754
Impacted Files Coverage Δ
datalad/distribution/tests/test_update.py 0% <ø> (-100%) ⬇️
datalad/distribution/tests/test_create_github.py 0% <ø> (-90.91%) ⬇️
datalad/distribution/tests/test_create.py 0% <0%> (-100%) ⬇️
datalad/distribution/tests/test_dataset_binding.py 0% <0%> (-100%) ⬇️
datalad/distribution/uninstall.py 30% <0%> (-69.38%) ⬇️
datalad/interface/tests/test_save.py 0% <0%> (-100%) ⬇️
datalad/interface/tests/test_utils.py 0% <0%> (-100%) ⬇️
datalad/distribution/add.py 67.36% <100%> (-27.37%) ⬇️
datalad/api.py 62.96% <100%> (-37.04%) ⬇️
datalad/tests/test_constraints.py 100% <100%> (ø) ⬆️
... and 205 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 9a8be5e...55689ab. Read the comment docs.

mih and others added some commits Feb 17, 2017

RF: Use better_wraps with @datasetmethod
RF: Fix datasetmethods' docstrings at the same time we do it for the datalad.api functions
NF: Quick demo for result eval, error detection, result rendering
This can work as a generator all the way up (good when processing a long
list of things that one wants to act upon from the outside).
Rendering of results can be triggered via config (likely should become
a general switch in cmdline API).

TODO merge datalad.api result rendering functionality into this
machinery to get rid of the "underscore" functions.

@mih mih force-pushed the mih:rf-returnvalues branch from d17de33 to 0c29ef4 Mar 1, 2017

@mih

This comment has been minimized.

Member

mih commented Mar 1, 2017

Rebased on master

@mih mih force-pushed the mih:rf-returnvalues branch from b3aebea to 01d758c Mar 2, 2017

@mih

This comment has been minimized.

Member

mih commented Mar 2, 2017

Test seem to pass now. Remaining failure is due to the unresolved PY3 issue from #1348.

mih and others added some commits Mar 2, 2017

BF/RF: docstrings for Dataset methods didn't work in PY3
- RF: use wrapt.decorator style for @datasetmethod and @eval_results
- RF: Remove docstring manipulation of Dataset methods during import of datalad.api altogether, since we don't need it anymore
ENH: Proposing a mechanism for a switch for eval_results, that can be…
… accessed from within eval_results without polluting the signature of the decorated command function
BF: `update` never reported to not act on "unavailable" paths
It just silently ignored them without error codes. Now reported and
evaluated.

mih added some commits Mar 16, 2017

RF+NF: Fixup and robustify tests with new helpers
Complete result helper set is now:

- assert_status (now with multi-status support)
- assert_message (test for message template identity)
- assert_in_results (any combination of properties possible)
- assert_not_in_results (like above but in inverted)
- assert_result_count (quick test how many results match given props)
- assert_fields_equal (check for any field equivalence, more general
    than assert_message)
RF: assert_field_values -> assert_result_values_equal
and put result sequence arg first like the other helpers have it.
@mih

This comment has been minimized.

Member

mih commented Mar 17, 2017

@yarikoptic @bpoldrack Apart form the following issues this PR is done IMHO:

  1. this weird test crash on travis, OSX, and nd90 (but nowhere else, including my local machine that is nd90+ and also runs SSH tests etc...) -- I was hoping we could make an attempt to dissect this today
  2. not all commands have been converted. However, many have been, this PR is complicated enough, and all interfaces (Python API, cmdline) and interface bridges are exercised in the PR (i.e. install(get) with old-style outside, update(get) both new-style). I will leave further conversions to the moment when we touch the remaining commands next.
@mih

This comment has been minimized.

Member

mih commented Mar 17, 2017

Ah forgot one: get needs RF for its tailored renderer. But it is actually a more fundamental issue: #1380

@mih

This comment has been minimized.

Member

mih commented Mar 19, 2017

I am reverting the merge of recent master, as it breaks this PR (see bottom of #1370).

@mih mih force-pushed the mih:rf-returnvalues branch from 6af75e8 to 55689ab Mar 19, 2017

@mih

This comment has been minimized.

Member

mih commented Mar 19, 2017

Story continues at #1393

@mih mih closed this Mar 19, 2017

mih added a commit that referenced this pull request Mar 21, 2017

Merge pull request #1393 from mih/rf-returnvalues
Return value revolution (#1350 continued)

@mih mih referenced this pull request Mar 21, 2017

Merged

RF: return values (#1350 continued) #1409

46 of 53 tasks complete

yarikoptic added a commit that referenced this pull request May 6, 2017

Merge pull request #1409 from datalad/rf-returnvalues
RF: return values (#1350 continued)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment