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

DOC: Fixed documentation builder to allow for overriding opts in eval… #4480

Merged
merged 8 commits into from Jun 19, 2020

Conversation

andycon
Copy link
Contributor

@andycon andycon commented May 4, 2020

…_defaults. Closes #2762

Edited the decorator function build_doc in interface/base.py, so that when a class call function specifies another default value other than that in eval_defaults/eval_params from common_opts, the specified default properly overwrites the generic default value, and repacked the default value in the doc string.

@codecov
Copy link

@codecov codecov bot commented May 4, 2020

Codecov Report

Merging #4480 into master will increase coverage by 40.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4480       +/-   ##
===========================================
+ Coverage   49.64%   89.64%   +40.00%     
===========================================
  Files         288      288               
  Lines       39013    39020        +7     
===========================================
+ Hits        19367    34980    +15613     
+ Misses      19646     4040    -15606     
Impacted Files Coverage Δ
datalad/interface/base.py 91.22% <100.00%> (+0.07%) ⬆️
datalad/interface/tests/test_utils.py 100.00% <100.00%> (ø)
datalad/support/constraints.py 87.66% <0.00%> (+0.44%) ⬆️
datalad/core/local/tests/test_create.py 100.00% <0.00%> (+0.44%) ⬆️
datalad/local/subdatasets.py 95.65% <0.00%> (+0.86%) ⬆️
datalad/core/local/diff.py 95.29% <0.00%> (+1.17%) ⬆️
datalad/support/external_versions.py 96.07% <0.00%> (+1.30%) ⬆️
datalad/dochelpers.py 87.40% <0.00%> (+1.48%) ⬆️
datalad/local/tests/test_copy_file.py 100.00% <0.00%> (+1.58%) ⬆️
datalad/support/tests/test_network.py 100.00% <0.00%> (+1.79%) ⬆️
... and 180 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 1d11ca9...9940af0. Read the comment docs.

Copy link
Member

@yarikoptic yarikoptic left a comment

Thank you @andycon ! I left a recommendation on code composition.
Could you also craft a unit-test which would fail without this fix but pass when fix is applied?

# update defaults for any opts in spec that can override eval_defaults
overriders = dict()
for opt in spec.keys():
if opt in eval_params.keys():
Copy link
Member

@yarikoptic yarikoptic May 4, 2020

Choose a reason for hiding this comment

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

no need for an explicit .keys() in either of above two lines or below

if opt in eval_params.keys():
overriders[opt] = spec[opt]
for opt in overriders.keys():
xx = spec.pop(opt)
Copy link
Member

@yarikoptic yarikoptic May 4, 2020

Choose a reason for hiding this comment

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

since do get .poped at the end anyways, .pop could happen at the point of assigning to overriders. And entire code block would be more concise and descriptive as a dict comprehension, such as

overriders = {
  opt: spec.pop(opt)
  for opt in spec
  if opt in eval_params
}

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 4, 2020

datalad-container failure (in running wtf!) -- I wonder if it could be related since I do not remember it seeing failing otherwise recently:

Run datalad wtf
[ERROR] value is not one of ('ignore', 'continue', 'stop') [constraints.py:__call__:289] (ValueError) 
##[error]Process completed with exit code 1.

that is getting deep into the weeds ;-/

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 4, 2020

datalad-container failure (in running wtf!) -- I wonder if it could be related since I do not remember it seeing failing otherwise recently:

It is related. I have a review coming.

Copy link
Contributor

@kyleam kyleam left a comment

Thanks for the PR.

Subject: [PATCH] DOC: Fixed documentation builder to allow for overriding opts
in eval_defaults. Closes #2762

Style/convention nit-pick: Please move the "Closes ..." bit into the body of the commit message. (Even without the "Closes ..." statement, that's getting long for a subject line. )

It'd be good to include a description of the problem and some information about your analysis/fix in the commit message body. What you say in the PR's description would be a good start:

Edited the decorator function build_doc in interface/base.py, so that when a class call function specifies another default value other than that in eval_defaults/eval_params from common_opts, the specified default properly overwrites the generic default value, and repacked the default value in the doc string.

From that, I take away that the problem is that, whenever a class overrides the default for something in eval_params, the Python documentation shows an incorrect default.

In that same sentence is the proposed solution, which seems to boil down to making build_doc check for class overrides. I haven't worked with the datalad.interface stuff frequently enough to keep it all fresh in my head, but that sounds like the right spot.

datalad/distribution/siblings.py | 3 ++-
datalad/interface/base.py | 10 ++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)

Based on reading the above, I'm surprised to see this. It sounded like only an interface module should have to change, not an individual command like siblings. And if each command did need to change, I'd expect to at least see every command that uses a tailored renderer here. So, without having yet looked at proposed code change, it seems like something's not quite right here.

When I test out your PR with python -m pydoc datalad.api.siblings, I see the default for result_renderer is reported as tailored, so it does look like this resolves the specific issue the prompted this PR (gh-2762). However, if I try with another command that I know has a tailored result renderer (python -m pydoc datalad.api.diff), the default is still reported as None.

datalad/interface/base.py Outdated Show resolved Hide resolved
kyleam added a commit to kyleam/datalad-container that referenced this issue May 4, 2020
containers-run builds ContainersRun._params_ from Run._params_ to
extend run's interface with container-specific parameters.  However,
this is unintentionally populating ContainersRun._params_ with
parameters from eval_params because build_doc() modifies Run._params_
in place.

Having eval_param keys in ContainersRun._params_ doesn't currently
cause any issues because build_doc() does a blanket update of _params_
with eval_params, but a proposed change to build_doc() exposed the
issue (datalad/datalad#4480).  That patch will likely be rewritten in
a way that doesn't trigger the issue, but, in hopes of avoiding
similar issues in the future, let's rework ContainersRun handling to
not depend on internal details of how build_doc() works.
kyleam added a commit to kyleam/datalad that referenced this issue May 4, 2020
containers-run builds ContainersRun._params_ from Run._params_ to
extend run's interface with container-specific parameters.  In doing
so, it (presumably unintentionally) populates ContainersRun._params_
with parameters from eval_params because build_doc() modifies
Run._params_ in place.

Having eval_param keys in ContainersRun._params_ doesn't currently
cause any issues because build_doc() does a blanket update of _params_
with eval_params, but a proposed change to build_doc() in dataladgh-4480
shows how this can fail.

Explicitly document this behavior to make it clear to callers that
they can rely on it and to prevent future build_doc() tweakers from
unknowingly changing it.

Supersedes datalad/datalad-container#119.
This should Close datalad#2762

My previous fix added results_renderer to the _params_ in interface.Siblings which wasn't necessary as Kyle pointed out.
I have incorporated his suggestions, and it seems to work. Thanks
Copy link
Contributor Author

@andycon andycon left a comment

Thanks Kyle. This should clears things up.

@andycon
Copy link
Contributor Author

@andycon andycon commented May 4, 2020

From that, I take away that the problem is that, whenever a class overrides the default for something in eval_params, the Python documentation shows an incorrect default.

@andycon andycon closed this May 4, 2020
@andycon andycon deleted the iss2762 branch May 4, 2020
@kyleam
Copy link
Contributor

@kyleam kyleam commented May 4, 2020

Andy: Thanks for the updates. Why did you delete your branch and close this PR?

@andycon
Copy link
Contributor Author

@andycon andycon commented May 4, 2020

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 4, 2020

I did not mean to do that. I am still learning how to use the wed interface on GitHub. I will send a new PR now.

Okay, no worries. Either way works, but you may just be able to restore your branch and reopen this PR (via github buttons).

@andycon
Copy link
Contributor Author

@andycon andycon commented May 4, 2020

@andycon andycon restored the iss2762 branch May 4, 2020
@andycon andycon reopened this May 4, 2020
kyleam
kyleam approved these changes May 4, 2020
Copy link
Contributor

@kyleam kyleam left a comment

Thanks, the updates look good to me.

To get a better idea of what the overall changes are, I ran the following script with master (73058ae) and this PR (d17a0cd):

import pydoc
from datalad.interface.base import get_api_name
from datalad.interface.base import get_interface_groups

for group, _, specs in sorted(get_interface_groups(), key=lambda x: x[1]):
    for spec in specs:
        command = get_api_name(spec)
        pydoc.help("datalad.api." + command)
Here are the differences:
diff --git a/pydoc-pre b/pydoc-post
index 8088897..ab880d2 100644
--- a/pydoc-pre
+++ b/pydoc-post
@@ -106,7 +106,8 @@ datalad.api.create = __call__(path=None, initopts=None, force=False, description
       callable, and is only returned if the callable's return value does
       not evaluate to False or a ValueError exception is raised. If the
       given callable supports `**kwargs` it will additionally be passed
-      the keyword arguments of the original API call. [Default: None]
+      the keyword arguments of the original API call. [Default:
+      constraint:(action:{create} and status:{ok, notneeded})]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
       format of return value rendering on stdout. [Default: None]
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
@@ -116,12 +117,12 @@ datalad.api.create = __call__(path=None, initopts=None, force=False, description
       transformation of the result value. This is mostly useful for top-
       level command invocations that need to provide the results in a
       particular format. Instead of a callable, a label for a pre-crafted
-      result transformation can be given. [Default: None]
+      result transformation can be given. [Default: 'datasets']
     return_type : {'generator', 'list', 'item-or-list'}, optional
       return value behavior switch. If 'item-or-list' a single value is
       returned instead of a one-item return value list, or a list in case
       of multiple return values. `None` is return in case of an empty
-      list. [Default: 'list']
+      list. [Default: 'item-or-list']
 
 Help on function __call__ in datalad.api:
 
@@ -236,7 +237,8 @@ datalad.api.install = __call__(path=None, source=None, dataset=None, get_data=Fa
       callable, and is only returned if the callable's return value does
       not evaluate to False or a ValueError exception is raised. If the
       given callable supports `**kwargs` it will additionally be passed
-      the keyword arguments of the original API call. [Default: None]
+      the keyword arguments of the original API call. [Default: <function
+      is_result_matching_pathsource_argument at 0x7f5698338598>]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
       format of return value rendering on stdout. [Default: None]
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
@@ -246,12 +248,13 @@ datalad.api.install = __call__(path=None, source=None, dataset=None, get_data=Fa
       transformation of the result value. This is mostly useful for top-
       level command invocations that need to provide the results in a
       particular format. Instead of a callable, a label for a pre-crafted
-      result transformation can be given. [Default: None]
+      result transformation can be given. [Default: 'successdatasets-or-
+      none']
     return_type : {'generator', 'list', 'item-or-list'}, optional
       return value behavior switch. If 'item-or-list' a single value is
       returned instead of a one-item return value list, or a list in case
       of multiple return values. `None` is return in case of an empty
-      list. [Default: 'list']
+      list. [Default: 'item-or-list']
 
 Help on function __call__ in datalad.api:
 
@@ -2059,7 +2062,7 @@ datalad.api.metadata = __call__(path=None, dataset=None, get_aggregates=False, r
       given callable supports `**kwargs` it will additionally be passed
       the keyword arguments of the original API call. [Default: None]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
-      format of return value rendering on stdout. [Default: None]
+      format of return value rendering on stdout. [Default: 'tailored']
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
       if given, each to-be-returned result status dictionary is passed to
       this callable, and its return value becomes the result instead. This
@@ -2336,7 +2339,7 @@ datalad.api.wtf = __call__(dataset=None, sensitive=None, sections=None, decor=No
       given callable supports `**kwargs` it will additionally be passed
       the keyword arguments of the original API call. [Default: None]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
-      format of return value rendering on stdout. [Default: None]
+      format of return value rendering on stdout. [Default: 'tailored']
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
       if given, each to-be-returned result status dictionary is passed to
       this callable, and its return value becomes the result instead. This
@@ -3046,7 +3049,7 @@ datalad.api.run_procedure = __call__(spec=None, dataset=None, discover=False, he
       given callable supports `**kwargs` it will additionally be passed
       the keyword arguments of the original API call. [Default: None]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
-      format of return value rendering on stdout. [Default: None]
+      format of return value rendering on stdout. [Default: 'tailored']
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
       if given, each to-be-returned result status dictionary is passed to
       this callable, and its return value becomes the result instead. This
@@ -3408,7 +3411,8 @@ datalad.api.clone = __call__(source, path=None, dataset=None, description=None,
       callable, and is only returned if the callable's return value does
       not evaluate to False or a ValueError exception is raised. If the
       given callable supports `**kwargs` it will additionally be passed
-      the keyword arguments of the original API call. [Default: None]
+      the keyword arguments of the original API call. [Default:
+      constraint:action:{install}]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
       format of return value rendering on stdout. [Default: None]
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
@@ -3418,12 +3422,13 @@ datalad.api.clone = __call__(source, path=None, dataset=None, description=None,
       transformation of the result value. This is mostly useful for top-
       level command invocations that need to provide the results in a
       particular format. Instead of a callable, a label for a pre-crafted
-      result transformation can be given. [Default: None]
+      result transformation can be given. [Default: 'successdatasets-or-
+      none']
     return_type : {'generator', 'list', 'item-or-list'}, optional
       return value behavior switch. If 'item-or-list' a single value is
       returned instead of a one-item return value list, or a list in case
       of multiple return values. `None` is return in case of an empty
-      list. [Default: 'list']
+      list. [Default: 'item-or-list']
 
 Help on function __call__ in datalad.api:
 
@@ -3614,7 +3619,7 @@ datalad.api.status = __call__(path=None, dataset=None, annex=None, untracked='no
       given callable supports `**kwargs` it will additionally be passed
       the keyword arguments of the original API call. [Default: None]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
-      format of return value rendering on stdout. [Default: None]
+      format of return value rendering on stdout. [Default: 'tailored']
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
       if given, each to-be-returned result status dictionary is passed to
       this callable, and its return value becomes the result instead. This
@@ -3726,7 +3731,7 @@ datalad.api.diff = __call__(path=None, fr='HEAD', to=None, dataset=None, annex=N
       given callable supports `**kwargs` it will additionally be passed
       the keyword arguments of the original API call. [Default: None]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
-      format of return value rendering on stdout. [Default: None]
+      format of return value rendering on stdout. [Default: 'tailored']
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
       if given, each to-be-returned result status dictionary is passed to
       this callable, and its return value becomes the result instead. This
@@ -3870,7 +3875,7 @@ datalad.api.siblings = __call__(action='query', dataset=None, name=None, url=Non
       given callable supports `**kwargs` it will additionally be passed
       the keyword arguments of the original API call. [Default: None]
     result_renderer : {'default', 'json', 'json_pp', 'tailored'} or None, optional
-      format of return value rendering on stdout. [Default: None]
+      format of return value rendering on stdout. [Default: 'tailored']
     result_xfm : {'datasets', 'successdatasets-or-none', 'paths', 'relpaths', 'metadata'} or callable or None, optional
       if given, each to-be-returned result status dictionary is passed to
       this callable, and its return value becomes the result instead. This

Those look sensible to me, so this seems good to go from my point of view, but @yarikoptic might push to get the test he requested :) Let me know if you need any pointers for getting started with that.

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 4, 2020

Those look sensible to me, so this seems good to go from my point of view,

hm...

[Default: <function is_result_matching_pathsource_argument at 0x7f5698338598>]

and

[Default:  constraint:(action:{create} and status:{ok, notneeded})]

seems to beg for some tune up?

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 4, 2020

seems to beg for some tune up?

I wouldn't mind if someone cared to improve the presentation, but surely those are preferable to showing an incorrect default?

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 5, 2020

That's true. And might be worth it's dedicated discussion. So I will let Andy decide either he is willing to add a unit test here to ensure (!) correct behavior going forward for this particular change. I think it would be very valuable.

@andycon
Copy link
Contributor Author

@andycon andycon commented May 5, 2020

I'm happy to write a unit test for this. The test should check all instances in the project where decorator @build_doc is applied and then ensure (!) that the values in the doc string match the actual values of the class attributes. This would only check attributes included in eval_defaults.

Does that sound right?

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 5, 2020

The test should check all instances in the project where decorator @build_doc is applied and then ensure (!) that the values in the doc string match the actual values of the class attributes. [...]
Does that sound right?

I think that sounds correct, though more ambitious than at least I had in mind. In my view checking the .__call_.__doc__ for a couple of commands that we know don't use the defaults would be sufficient.

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 5, 2020

yeah, would be great to have but too ambitious indeed. I would have created a new ad-hoc interface command for the sake of testing with known properties, and tested that one -- a "true" unit-test ;-) Actually we even have one already in https://github.com/datalad/datalad/blob/master/datalad/interface/tests/test_utils.py#L204 so you could just tune it up changing it so it would fail test without the introduced here fix, and then passing it with the fix

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 14, 2020

Hi @andycon . Please let us know your plan with this one -- test or no test? ;-) Cheers

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 19, 2020

@andycon Just a heads up: I'm about to bring in gh-4482, which will cause a conflict here. If you have any questions about resolving it, let me know.

andycon and others added 5 commits May 22, 2020
# Conflicts:
#	datalad/interface/base.py
added overriding parameters to existing test class TestUtils in  datalad/interface/tests/test_utils.py
passed my tests using :  DATALAD_TESTS_SSH=1 python -m nose -s -v datalad/interface/tests/test_utils.py
kyleam
kyleam approved these changes Jun 19, 2020
Copy link
Contributor

@kyleam kyleam left a comment

@andycon Thanks for adding the test. Looks good to me.

@@ -255,6 +258,10 @@ def test_eval_results_plus_build_doc():
assert_in("Parameters", doc1)
assert_in("It's a number", doc1)

# docstring shows correct overide values of defaults in eval_params
Copy link
Member

@yarikoptic yarikoptic Jun 19, 2020

Choose a reason for hiding this comment

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

Suggested change
# docstring shows correct overide values of defaults in eval_params
# docstring shows correct override values of defaults in eval_params

I will add this typo fix and merge/push directly.

Thanks @andycon !

@yarikoptic yarikoptic merged commit 9940af0 into datalad:master Jun 19, 2020
12 checks passed
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.

3 participants