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

Type annotate get_status_dict and note that we can pass Exception or CapturedException which is not subclass #7403

Merged
merged 4 commits into from
Jul 11, 2023

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Jun 2, 2023

I decided to start type annotating there instead of relying on numpy docstrings

Ref #6884

TODOs

  • properly type annotate that file and any related needed to be added to mypy checks etc

I decided to start type annotating there instead of relying on
numpy docstrings
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Jun 2, 2023
@jwodder
Copy link
Member

jwodder commented Jun 2, 2023

@yarikoptic What types are the fields of a status/result dict supposed to be?

@yarikoptic
Copy link
Member Author

@yarikoptic What types are the fields of a status/result dict supposed to be?

well, to be figured out from typical uses which could be grep'ed up.

@yarikoptic
Copy link
Member Author

@jwodder
Copy link
Member

jwodder commented Jun 6, 2023

@yarikoptic That documentation states that error_message is supposed to be a list of messages, yet get_status_dict() sometimes sets the error_message field of the result dict to CapturedException.message, which is a string, and annexjson2result() sets the field to a string as well.

@yarikoptic
Copy link
Member Author

Looking at the hits on "error_message" and because the name is singular
❯ git grep -p error_message
datalad/core/distributed/clone.py=def clone_dataset(
datalad/core/distributed/clone.py:            #error_message=ce.message,
datalad/core/local/tests/test_results.py=def test_generic_result_renderer():
datalad/core/local/tests/test_results.py:        (dict(message='funky', error_message='extra-funky'),
datalad/core/local/tests/test_status.py=def test_untracked_annex_query(path=None):
datalad/core/local/tests/test_status.py:        error_message='File unknown to git',
datalad/distributed/drop.py=def _drop_dataset(ds, paths, what, reckless, recursive, recursion_limit, jobs):
datalad/distributed/drop.py:            error_messages = r.get('error-messages')
datalad/distributed/drop.py:            if error_messages:
datalad/distributed/drop.py:                res['error_message'] = '\n'.join(
datalad/distributed/drop.py:                    m.strip() for m in error_messages
datalad/distributed/tests/test_drop.py=def test_drop_file_content(path=None, outside_path=None):
datalad/distributed/tests/test_drop.py:            error_message='File unknown to git',
datalad/distributed/tests/test_drop.py=def test_drop_uninit_annexrepo(origpath=None, path=None):
datalad/distributed/tests/test_drop.py:def test_drop_allkeys_result_contains_annex_error_messages(path=None):
datalad/distributed/tests/test_drop.py:    # message(s) to be present in the result record error_message
datalad/distributed/tests/test_drop.py:            error_message='git-annex error message here',
datalad/distributed/tests/test_ria_git_remote.py=def _test_bare_git_version_1(host, dspath, store):
datalad/distributed/tests/test_ria_git_remote.py:                        error_message='** Based on the location log, one.txt\n'
datalad/distributed/tests/test_ria_git_remote.py:                        error_message='** Based on the location log, subdir/two\n'
datalad/distributed/tests/test_ria_git_remote.py=def _test_bare_git_version_2(host, dspath, store):
datalad/distributed/tests/test_ria_git_remote.py:                        error_message='** Based on the location log, one.txt\n'
datalad/interface/results.py=def get_status_dict(action=None, ds=None, path=None, type=None, logger=None,
datalad/interface/results.py:                    error_message=None, **kwargs):
datalad/interface/results.py:    if error_message is not None:
datalad/interface/results.py:        d['error_message'] = error_message
datalad/interface/results.py:        if error_message is None and isinstance(exception, CapturedException):
datalad/interface/results.py:            d['error_message'] = exception.message
datalad/interface/results.py=def annexjson2result(d, ds, **kwargs):
datalad/interface/results.py:        res['error_message'] = '\n'.join(m.strip() for m in d['error-messages'])
datalad/interface/utils.py=def generic_result_renderer(res):
datalad/interface/utils.py:                res['error_message'][0] % res['error_message'][1:]
datalad/interface/utils.py:                if isinstance(res['error_message'], tuple) else res[
datalad/interface/utils.py:                    'error_message']), ac.RED)
datalad/interface/utils.py:            if res.get('error_message', None) and res.get('status', None) != 'ok' else ''))
datalad/runner/exception.py=class CommandError(RuntimeError):
datalad/runner/exception.py:                to_str += _format_json_error_messages(
datalad/runner/exception.py:def _format_json_error_messages(recs: list[dict]) -> str:
datalad/runner/tests/test_exception.py=from ..exception import (
datalad/runner/tests/test_exception.py:    _format_json_error_messages,
datalad/runner/tests/test_exception.py=def test_format_error_with_duplicates() -> None:
datalad/runner/tests/test_exception.py:    result = _format_json_error_messages(json_objects)
datalad/runner/tests/test_exception.py:    result = _format_json_error_messages(json_objects + json_objects)
datalad/runner/tests/test_exception.py=def test_format_no_errors() -> None:
datalad/runner/tests/test_exception.py:    result = _format_json_error_messages(json_objects)
datalad/runner/tests/test_exception.py:    result = _format_json_error_messages(json_objects + json_objects)
datalad/support/annexrepo.py=class AnnexRepo(GitRepo, RepoInterface):
datalad/support/annexrepo.py:                rec['error_message'] = '\n'.join(m.strip() for m in j['error-messages'])
docs/source/design/result_records.rst=A string label categorizing the state of an entity. Common values are:
docs/source/design/result_records.rst:``error_message``

it seems that it is always a string and documentation might be wrong/needs fixing. Did you spot where it was set to a list?

@jwodder
Copy link
Member

jwodder commented Jun 6, 2023

@yarikoptic There's at least a provision for if error_message is set to a tuple:

err=ac.color_word(' [{}]'.format(
res['error_message'][0] % res['error_message'][1:]
if isinstance(res['error_message'], tuple) else res[
'error_message']), ac.RED)

@yarikoptic
Copy link
Member Author

I guess it was added to mimic the behavior of "message" treatment which indeed often used as a tuple (string_interpolation_template: str, *string_interpolation_params). Let's allow for both pure string and a tuple

@jwodder jwodder marked this pull request as ready for review June 6, 2023 16:58
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Patch coverage: 94.28% and project coverage change: +2.65 🎉

Comparison is base (13aaeeb) 88.76% compared to head (a991073) 91.42%.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7403      +/-   ##
==========================================
+ Coverage   88.76%   91.42%   +2.65%     
==========================================
  Files         327      325       -2     
  Lines       44693    43399    -1294     
  Branches     5924     5818     -106     
==========================================
+ Hits        39671    39676       +5     
+ Misses       5007     3707    -1300     
- Partials       15       16       +1     
Impacted Files Coverage Δ
datalad/support/gitrepo.py 91.95% <ø> (-0.08%) ⬇️
datalad/support/tests/test_repo_save.py 100.00% <ø> (ø)
datalad/interface/results.py 95.13% <93.10%> (-1.19%) ⬇️
datalad/core/local/save.py 98.92% <100.00%> (+0.01%) ⬆️

... and 59 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yarikoptic
Copy link
Member Author

@jwodder is working on more annotations, so merging this would be beneficial since it does help to pick up possible typing snafus. I will merge tomorrow unless other @datalad/developers chime in with objections/concerns.

@yarikoptic yarikoptic requested a review from mih June 9, 2023 14:22
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

Just commenting on Exception vs CapturedException: I believe the latter was primarily invented for passing to get_result_dict(). A regular exception binds a traceback and the long-lived nature of results caused complex/incomprehensible behavior when interaction with other parts of datalad that bind ressources. I have not been in touch with that, only @yarikoptic and primarily @bpoldrack , I believe. AFAIR it was open files staying open and processes not ending.

Unless there is a good reason to risk such regressions, I'd stay away from it.

@yarikoptic
Copy link
Member Author

Unless there is a good reason to risk such regressions, I'd stay away from it.

I am not sure what you @mih have in mind since this PR AFAIK just adds types annotations -- it is just a matter of current state that we could pass Exception or CapturedException, this PR should not introduce any functionality changes (besides few typing fixups detected while type checking).

@yarikoptic yarikoptic changed the title BF: we can pass Exception or CapturedException which is not subclass Type annotate get_status_dict and note that we can pass Exception or CapturedException which is not subclass Jul 11, 2023
@yarikoptic yarikoptic added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jul 11, 2023
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jul 11, 2023
Copy link
Member

@adswa adswa left a comment

Choose a reason for hiding this comment

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

@yarikoptic asked me to take a look at this at the devcall, and while I'm not very knowledgeable about type annotations, I did not spot anything conceptually wrong with the PR, so I approve.

@yarikoptic
Copy link
Member Author

thank you @adswa ! neuroimaging CI failed due to some

Run bash <(wget -q -O- http://neuro.debian.net/_files/neurodebian-travis.sh)
+ sudo apt-get update -qq
E: Failed to fetch http://archive.ubuntu.com/ubuntu/dists/jammy-updates/main/binary-amd64/Packages.xz  File has unexpected size (778464 != 7785[9](https://github.com/datalad/datalad/actions/runs/5520860977/jobs/10068112163?pr=7403#step:2:10)2). Mirror sync in progress? [IP: 185.125.190.39 80]
   Hashes of expected file:
    - Filesize:778592 [weak]
    - SHA256:62f3789e16eaa4053f42aa290fdee9db4bae155f3483d01d0ce2527233b04185
    - SHA1:232a84c47d11ede65192f6a5a1938e23544114f2 [weak]
    - MD5Sum:f2eab7b034e5df6d6701861208f6562b [weak]
   Release file created at: Tue, 11 Jul 2023 13:30:51 +0000
E: Some index files failed to download. They have been ignored, or old ones used instead.
Error: Process completed with exit code [10](https://github.com/datalad/datalad/actions/runs/5520860977/jobs/10068112163?pr=7403#step:2:11)0.

and since the rest is green -- let's proceed!

@yarikoptic yarikoptic merged commit 5bae877 into datalad:maint Jul 11, 2023
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.19.3

@yarikoptic yarikoptic deleted the enh-types branch February 2, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants