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

Improve uptodateness of config reports across manager instances #7301

Merged
merged 7 commits into from
Mar 25, 2023

Conversation

mih
Copy link
Member

@mih mih commented Feb 23, 2023

The issue is described in #7299. This change set contains three commits

  • one documents both aspects of the issue in a test
  • one reduced the amount of manager synchronization that needs to be done by no longer handing out custom ConfigManager instances for Datasets without a repository
  • one now causes an update of the global datalad.cfg instance, whenever any ConfigManager updated the global configuration scope

Closes #7299

@mih mih added the semver-patch Increment the patch version when merged label Feb 23, 2023
@adswa adswa added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Feb 23, 2023
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Feb 23, 2023
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Seems to be kosher to me. Left only one ignorable suggestion

datalad/config.py Outdated Show resolved Hide resolved
changelog.d/pr-7301.md Outdated Show resolved Hide resolved
@adswa
Copy link
Member

adswa commented Feb 27, 2023

Crippled FS has a rerun test failure that looks unrelated:

Click to expand
=================================== FAILURES ===================================
______________________ test_rerun_fastforwardable_mutator ______________________

path = '/crippledfs/datalad_temp_test_rerun_fastforwardable_mutator1qtr9bo5'

    @slow
    @with_tempfile(mkdir=True)
    def test_rerun_fastforwardable_mutator(path=None):
        ds = Dataset(path).create()
        # keep direct repo accessor to speed things up
        ds_repo = ds.repo
        ds_repo.checkout(DEFAULT_BRANCH, options=["-b", "side"])
        ds.run("echo foo >>foo")
        ds_repo.checkout(DEFAULT_BRANCH)
        ds_repo.merge("side", options=["-m", "Merge side", "--no-ff"])
        #  o                 c_n
        #  |\
        #  | o               b_r
        #  |/
        #  o                 a_n
    
>       ds.rerun(since="", onto=DEFAULT_BRANCH + "^2")

/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/local/tests/test_rerun_merges.py:105: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/distribution/dataset.py:507: in apply_func
    return f(*args, **kwargs)
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/interface/base.py:773: in eval_func
    return return_func(*args, **kwargs)
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/interface/base.py:763: in return_func
    results = list(results)
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/interface/base.py:885: in _execute_command_
    allkwargs):
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/interface/utils.py:319: in _process_results
    for res in results:
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/local/rerun.py:278: in __call__
    for res in handler(ds, results):
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/local/rerun.py:453: in _rerun
    new_parents[1:])
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/dataset/gitrepo.py:461: in call_git
    keep_ends=True))
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/dataset/gitrepo.py:522: in call_git_items_
    sep=sep):
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/dataset/gitrepo.py:355: in _generator_call_git
    for file_no, content in generator:
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/_collections_abc.py:317: in __next__
    return self.send(None)
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/runner/nonasyncrunner.py:97: in send
    return self._locked_send(message)
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/runner/nonasyncrunner.py:122: in _locked_send
    self._check_result()
/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/runner/nonasyncrunner.py:93: in _check_result
    self.runner._check_result()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <datalad.runner.nonasyncrunner.ThreadedRunner object at 0x7f8aa8ebc790>

    def _check_result(self):
        if self.exception_on_error is True:
            if self.return_code not in (0, None):
                protocol = self.protocol
                decoded_output = {
                    source: protocol.fd_infos[fileno][1].decode(protocol.encoding)
                    for source, fileno in (
                        ("stdout", protocol.stdout_fileno),
                        ("stderr", protocol.stderr_fileno))
                    if protocol.fd_infos[fileno][1] is not None
                }
                raise CommandError(
                    cmd=self.cmd,
                    code=self.return_code,
                    stdout=decoded_output.get("stdout", None),
>                   stderr=decoded_output.get("stderr", None)
                )
E               datalad.runner.exception.CommandError: CommandError: 'git -c diff.ignoreSubmodules=none merge -m 'Merge side
E               ' --no-ff --allow-unrelated-histories 4f5f08136b567a66406c75ed68c50ed494ad9c5c' failed with exitcode 128 [err: 'error: Your local changes to the following files would be overwritten by merge:
E               	foo
E               Please commit your changes or stash them before you merge.
E               Aborting']

/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/datalad/runner/nonasyncrunner.py:299: CommandError
=============================== warnings summary ===============================
tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:901: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete is using nose-specific method: `setup(self)`
  To remove this warning, rename it to `setup_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    fixture_result = next(generator)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:917: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete is using nose-specific method: `teardown(self)`
  To remove this warning, rename it to `teardown_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    next(it)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_archive_leading_dir
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:901: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_archive_leading_dir is using nose-specific method: `setup(self)`
  To remove this warning, rename it to `setup_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    fixture_result = next(generator)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_archive_leading_dir
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:917: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_archive_leading_dir is using nose-specific method: `teardown(self)`
  To remove this warning, rename it to `teardown_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    next(it)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:901: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop is using nose-specific method: `setup(self)`
  To remove this warning, rename it to `setup_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    fixture_result = next(generator)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:917: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop is using nose-specific method: `teardown(self)`
  To remove this warning, rename it to `teardown_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    next(it)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop_subdir
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:901: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop_subdir is using nose-specific method: `setup(self)`
  To remove this warning, rename it to `setup_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    fixture_result = next(generator)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop_subdir
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:917: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_add_delete_after_and_drop_subdir is using nose-specific method: `teardown(self)`
  To remove this warning, rename it to `teardown_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    next(it)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_override_existing_under_git
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:901: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_override_existing_under_git is using nose-specific method: `setup(self)`
  To remove this warning, rename it to `setup_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    fixture_result = next(generator)

tests/test_add_archive_content.py::TestAddArchiveOptions::test_override_existing_under_git
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/_pytest/fixtures.py:917: PytestRemovedIn8Warning: Support for nose tests is deprecated and will be removed in a future release.
  tests/test_add_archive_content.py::TestAddArchiveOptions::test_override_existing_under_git is using nose-specific method: `teardown(self)`
  To remove this warning, rename it to `teardown_method(self)`
  See docs: https://docs.pytest.org/en/stable/deprecations.html#support-for-tests-written-for-nose
    next(it)

tests/test_ria_basics.py::test_initremote_basic_httpsurl
  /opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/urllib3/connection.py:465: SubjectAltNameWarning: Certificate for localhost has no `subjectAltName`, falling back to check for a `commonName` for now. This feature is being removed by major browsers and deprecated by RFC 2818. (See https://github.com/urllib3/urllib3/issues/497 for details.)
    SubjectAltNameWarning,

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED ../tests/test_rerun_merges.py::test_rerun_fastforwardable_mutator - datalad.runner.exception.CommandError: CommandError: 'git -c diff.ignoreSubmodules=none merge -m 'Merge side
' --no-ff --allow-unrelated-histories 4f5f08136b567a66406c75ed68c50ed494ad9c5c' failed with exitcode 128 [err: 'error: Your local changes to the following files would be overwritten by merge:
	foo
Please commit your changes or stash them before you merge.
Aborting']
= 1 failed, 329 passed, 63 skipped, 1 deselected, 1 xpassed, 11 warnings in 1477.69s (0:24:37) =

The failure on MacOS is #7286, and two of the Travis errors are installation failures. However, there is one Travis error that seems relevant to this change set:

___________________________ test_git_config_warning ____________________________

[gw0] linux -- Python 3.8.6 /tmp/dl-miniconda-jjb5bm54/bin/python
path = '/tmp/datalad_temp_tree_test_git_config_warning099jf49k'
    @with_tree(tree={})
    def test_git_config_warning(path=None):
        if 'GIT_AUTHOR_NAME' in os.environ:
            raise SkipTest("Found existing explicit identity config")

        # Note: An easier way to test this, would be to just set GIT_CONFIG_GLOBAL
        # to point somewhere else. However, this is not supported by git before
        # 2.32. Hence, stick with changed HOME in this test, but be sure to unset a
        # possible GIT_CONFIG_GLOBAL in addition.
        patched_env = os.environ.copy()
        patched_env.pop('GIT_CONFIG_GLOBAL', None)
        patched_env.update(get_home_envvars(path))
        with chpwd(path), \
                patch.dict('os.environ', patched_env, clear=True), \
                swallow_logs(new_level=30) as cml:
            # no configs in that empty HOME
            from datalad.api import Dataset
            from datalad.config import ConfigManager

            # reach into the class and disable the "checked" flag that
            # has already been tripped before we get here
            ConfigManager._checked_git_identity = False
            Dataset(path).config.reload()
>           assert_in("configure Git before", cml.out)
../datalad/tests/test_base.py:94: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

first = 'configure Git before', second = '', msg = None
    def assert_in(first, second, msg=None):
        if msg is None:
>           assert first in second
E           AssertionError: assert 'configure Git before' in ''
../datalad/tests/utils_pytest.py:97: AssertionError

Edit: I can reproduce this failure locally on this branch, and not on maint.

@adswa
Copy link
Member

adswa commented Feb 27, 2023

I've looked into the failure, and at the time in the test where we expect an unset Git identity, the Git identity is already set and keeps being set after a config manager reload:

(Pdb) Dataset(path).config['user.name']
'DataLad Tester'
(Pdb) n
> /home/adina/repos/datalad/datalad/tests/test_base.py(94)test_git_config_warning()
-> Dataset(path).config.reload()
(Pdb) n
> /home/adina/repos/datalad/datalad/tests/test_base.py(95)test_git_config_warning()
-> assert_in("configure Git before", cml.out)
(Pdb) Dataset(path).config['user.name']
'DataLad Tester'

I suppose this is not a bug, but rather a side effect of the bug this PR fixes. Since the dataset is fresh and doesn't have a config manager yet, this PR makes the config manager return the global one - which already has a Git identity set.
I believe that the test tests that an unset Git identity only issues a warning instead of raising an error (ref fc5a54b). What would be a better way to test this? Unsetting the user.name and ``user.email` from the Config manager?

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Thx, @mih.

Suggestions by @adswa and @yarikoptic make sense to me, otherwise I think this right and ready.

@bpoldrack
Copy link
Member

bpoldrack commented Mar 24, 2023

@adswa

I think the trouble in that test is two-fold:

  • the changed behavior of reload right before the assertion
  • the fact that this tests a bound ConfigManager to begin with. The proper way is having this test a fresh, independent ConfigManager instance (and set its identity attribute accordingly), since the tested behavior has nothing to do with a Dataset.

Edit: Will shortly push accordingly. (Done. I hope)

mih and others added 7 commits March 24, 2023 16:59
Also ping datalad#7300 re diversity of exception behavior for one and the same
issue.
This has seemlingly no purpose or use case. It only complicates the
synchronization between, here needlessly, many manager instances.

This addressed half of datalad#7299
...whenever the 'global' configuration scope was modified.
Theoretically, this should also cover a 'system' scope, but presently
`ConfigManager` does not recognize or support that.

Closes datalad#7299
Use an independent `ConfigManager` instance to test the desire behavior,
instead of one that is bound to a `Dataset` instance and thereby
indirectly to `datalad.cfg`.
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Adina Wagner <adina.wagner@t-online.de>
@yarikoptic
Copy link
Member

approved so let's merge. I just rebased on top of current maint to get a fresh run since prior one was plagued with redness. will merge if all good

@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.60 🎉

Comparison is base (362834f) 88.08% compared to head (97735d3) 90.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7301      +/-   ##
==========================================
+ Coverage   88.08%   90.68%   +2.60%     
==========================================
  Files         327      327              
  Lines       44592    44609      +17     
  Branches     5906     5907       +1     
==========================================
+ Hits        39278    40453    +1175     
+ Misses       5299     4141    -1158     
  Partials       15       15              
Impacted Files Coverage Δ
datalad/config.py 97.51% <100.00%> (+0.23%) ⬆️
datalad/distribution/dataset.py 96.34% <100.00%> (ø)
datalad/tests/test_base.py 100.00% <100.00%> (ø)
datalad/tests/test_config.py 99.74% <100.00%> (+0.01%) ⬆️

... and 48 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ 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

travis and appveyor green -- some flukes on OSX I will not even look into...

@yarikoptic yarikoptic merged commit 360713d into datalad:maint Mar 25, 2023
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.18.3

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.

Non-installed Dataset instance is using a separate, but non-global config manager
5 participants