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

Enhance run() placeholder substitutions to honor configuration defaults #7509

Open
wants to merge 3 commits into
base: maint
Choose a base branch
from

Conversation

mih
Copy link
Member

@mih mih commented Oct 13, 2023

Previously, run() would not recognize configuration defaults for placeholder substitution. This means that any placeholders globally declared in datalad.interface.common_cfg, or via register_config() in DataLad extensions would not be effective.

This changeset makes run's format_command() helper include such defaults explicitly, and thereby enable the global declaration of substitution defaults.

Moreoever a {python} placeholder is now defined via this mechanism, and points to the value of sys.executable by default. This particular placeholder was found to be valueable for improving the portability of run-recording across (specific) Python versions, or across different (virtual) environments. See
datalad/datalad-container#224 for an example use case.

A corresponding test is included.

The ability to keep run-records paramterizable, in particular, for interpreters can also aid longevity (think platform-specific choices to call a system Python executable python3 for some years, and then switch back.

Related datalad/datalad-container#250

…ults

Previously, `run()` would not recognize configuration defaults for
placeholder substitution. This means that any placeholders globally
declared in `datalad.interface.common_cfg`, or via `register_config()`
in DataLad extensions would not be effective.

This changeset makes run's `format_command()` helper include such
defaults explicitly, and thereby enable the global declaration of
substitution defaults.

Moreoever a `{python}` placeholder is now defined via this mechanism,
and points to the value of `sys.executable` by default. This particular
placeholder was found to be valueable for improving the portability of
run-recording across (specific) Python versions, or across different
(virtual) environments. See
datalad/datalad-container#224 for an example
use case.

A corresponding test is included.

The ability to keep run-records paramterizable, in particular, for
interpreters can also aid longevity (think platform-specific choices
to call a system Python executable `python3` for some years, and then
switch back.

Related datalad/datalad-container#250
@mih mih added cmd-run/rerun Issues about the favorite command of ReproPeople semver-patch Increment the patch version when merged labels Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a96c51c) 91.56% compared to head (776e66b) 91.56%.
Report is 47 commits behind head on maint.

❗ Current head 776e66b differs from pull request most recent head 7b007cd. Consider uploading reports for the commit 7b007cd to get more accurate results

Files Patch % Lines
datalad/core/local/tests/test_run.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7509      +/-   ##
==========================================
- Coverage   91.56%   91.56%   -0.01%     
==========================================
  Files         325      325              
  Lines       43443    43457      +14     
  Branches     5827     5828       +1     
==========================================
+ Hits        39780    39791      +11     
- Misses       3648     3651       +3     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mih
Copy link
Member Author

mih commented Oct 13, 2023

MacOS failure looks unrelated (web remote).

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.

I need to understand it better: isn't this just a singular case of a larger common (?) pattern that in many (most?) cases we would actually like something like this for any Dataset.config access since dataset's config should be a "union" with system/user/env-overloads settings?

datalad/core/local/run.py Show resolved Hide resolved
@mih
Copy link
Member Author

mih commented Oct 15, 2023

I dont know if this is a common case. Typically, access that requires acknowledging a configurable default goes via obtain(). Here it is different, because it needs to look for matches and not for a specific item.

mih added a commit to mih/datalad-next that referenced this pull request Oct 18, 2023
Reported by @yarikoptic in
https://github.com/datalad/datalad/pull/7509/files#r1358426552

This changeset adds protection against processing non-substitution
configuration items.

It also reduces duplication a bit more.

In contrast to the change proposal in
datalad/datalad#7509 the (intermediate) set size
is minimized by using a early filter.
@yarikoptic yarikoptic added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Nov 8, 2023
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Nov 8, 2023
@yarikoptic
Copy link
Member

appveyor might have stalled (restarted).
[gw1] PASSED ../datalad/support/tests/test_sshconnector.py::test_bundle_invariance 
../datalad/support/tests/test_sshrun.py::test_exit_code CommandError: 'ssh -o ControlPath=/home/appveyor/.cache/datalad/sockets/bad871d9 datalad-test 'exit 42'' failed with exitcode 42
[gw1] XPASS ../datalad/support/tests/test_sshrun.py::test_exit_code 
../datalad/support/tests/test_sshrun.py::test_no_stdin_swallow 
[gw1] PASSED ../datalad/support/tests/test_sshrun.py::test_no_stdin_swallow 
../datalad/support/tests/test_sshrun.py::test_fancy_quotes 
[gw1] PASSED ../datalad/support/tests/test_sshrun.py::test_fancy_quotes 
../datalad/support/tests/test_sshrun.py::test_ssh_option 
[gw1] SKIPPED ../datalad/support/tests/test_sshrun.py::test_ssh_option 
../datalad/support/tests/test_sshrun.py::test_ssh_ipv4_6_incompatible 
[gw1] PASSED ../datalad/support/tests/test_sshrun.py::test_ssh_ipv4_6_incompatible 
../datalad/support/tests/test_sshrun.py::test_ssh_ipv4_6 ssh: Could not resolve hostname 127.0.0.1: Address family for hostname not supported
[gw1] PASSED ../datalad/support/tests/test_sshrun.py::test_ssh_ipv4_6 
../datalad/support/tests/test_stats.py::test_ActivityStats_basic 
[gw1] PASSED ../datalad/support/tests/test_stats.py::test_ActivityStats_basic 
../datalad/support/tests/test_stats.py::test_ActivityStats_comparisons 
[gw1] PASSED ../datalad/support/tests/test_stats.py::test_ActivityStats_comparisons 
../datalad/support/tests/test_stats.py::test_add 
[gw1] PASSED ../datalad/support/tests/test_stats.py::test_add 
../datalad/support/tests/test_status.py::test_FileStatus_basic 
[gw1] PASSED ../datalad/support/tests/test_status.py::test_FileStatus_basic 
../datalad/support/tests/test_vcr_.py::test_use_cassette_if_no_vcr 

OSX restarted... travis seems no more (I ended subscription but was hoping that opensource thing would kick in... will check later)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd-run/rerun Issues about the favorite command of ReproPeople semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants