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

ENH: add --cfg-proc to install, clone, and get #7477

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bpinsard
Copy link
Contributor

@bpinsard bpinsard commented Aug 16, 2023

Adds the --cfg-proc option to install command to allow running commands on dataset post-installation. see #7468
It runs procedure after recursive install.

It does not call the procedure recursively for now (recursion can be coded in the procedure itself).
Making it recursive would require way more changes to get and other parts of the code.

PR checklist

  • Write tests.
  • fix tests
  • refactor code for harmonious helper name/interface
  • @yarikoptic to add changelog entry later on

@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Patch coverage: 94.11% and project coverage change: +0.93% 🎉

Comparison is base (eeb7313) 90.61% compared to head (5e73715) 91.55%.

❗ Current head 5e73715 differs from pull request most recent head 0de85d5. Consider uploading reports for the commit 0de85d5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7477      +/-   ##
==========================================
+ Coverage   90.61%   91.55%   +0.93%     
==========================================
  Files         325      325              
  Lines       43405    43431      +26     
  Branches        0     5821    +5821     
==========================================
+ Hits        39333    39762     +429     
+ Misses       4072     3654     -418     
- Partials        0       15      +15     
Files Changed Coverage Δ
datalad/distribution/get.py 96.13% <ø> (+0.77%) ⬆️
datalad/core/distributed/clone.py 98.58% <75.00%> (-0.46%) ⬇️
datalad/core/local/create.py 99.30% <100.00%> (+0.71%) ⬆️
datalad/distribution/install.py 98.88% <100.00%> (+0.02%) ⬆️

... and 72 files with indirect coverage changes

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

@bpinsard
Copy link
Contributor Author

Maybe it needs to also be implemented for get when subsequently getting subdatasets. WDYT?

@yarikoptic
Copy link
Member

It does not call the procedure recursively for now (recursion can be coded in the procedure itself).

I do think that procedure better to operate non-recursively and then indeed it be get to actually gain that option first -- the "older style" install is pretty much a wrapper around get and some folks are just using datalad clone and datalad get tandem instead of install.

@yarikoptic yarikoptic added semver-minor Increment the minor version when merged CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Aug 16, 2023
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Aug 16, 2023
@bpinsard bpinsard changed the title ENH: add --cfg-proc to install ENH: add --cfg-proc to install/get Aug 16, 2023
@bpinsard bpinsard changed the title ENH: add --cfg-proc to install/get ENH: add --cfg-proc to install/clone/get Aug 16, 2023
@yarikoptic
Copy link
Member

Sorry @bpinsard we didn't follow up on this one!
Could you please ideally rebase and force-push so we have a fresh runs of CIs -- and let's aim to see it merged/released.

Other @datalad/developers -- any feedback on this one? looks good and usefull to me.

@yarikoptic yarikoptic requested a review from mih March 6, 2024 18:45
@yarikoptic yarikoptic added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Mar 6, 2024
@yarikoptic yarikoptic changed the title ENH: add --cfg-proc to install/clone/get ENH: add --cfg-proc to install, clone, and get Mar 6, 2024
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Mar 6, 2024
@yarikoptic
Copy link
Member

changelog addition commit got lost, re-added the label. but CI failed to add that commit

remote: Permission to courtois-neuromod/datalad.git denied to yarikoptic-gitmate.
fatal: unable to access 'https://github.com/courtois-neuromod/datalad/': The requested URL returned error: 403

@bpinsard please permit other maintainers to contribute (I think it is the case why it failed) to this PR (checkmark on the side to the right above IIRC). Or produce it manually using scriv tool.

@bpinsard
Copy link
Contributor Author

bpinsard commented Mar 6, 2024

Not sure what is wrong in the config, I am pretty sure that I simply forked the repo, what access should I grant and to who and where?

@yarikoptic
Copy link
Member

on the right under Paricipants - do you see smth like

image

? didn't look where generally that is configured in profile

@yarikoptic
Copy link
Member

tests fail BTW and need fixing, a sample from appveyor:

=========================================================================================================== short test summary info ============================================================================================================
FAILED ../datalad/distribution/tests/test_install.py::test_install_withcfg - TypeError: __call__() takes from 0 to 1 positional arguments but 2 positional arguments (and 1 keyword-only argument) were given
FAILED ../datalad/distribution/tests/test_install.py::test_recursive_install_withcfg - TypeError: __call__() takes from 0 to 1 positional arguments but 2 positional arguments (and 2 keyword-only arguments) were given

Comment on lines 558 to 562
def _procedures_exists(ds, cfg_proc):
# Check if specified cfg_proc(s) can be discovered, storing
# the results so they can be used when the time comes to run
# the procedure. If a procedure cannot be found, raise an
# error to prevent creating the dataset.
Copy link
Member

@yarikoptic yarikoptic Mar 6, 2024

Choose a reason for hiding this comment

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

I think this function should get into datalad/local/run_procedure.py to be along with other procedure support functions:

❯ git grep 'def _*get.*proc'
datalad/local/run_procedure.py:def _get_proc_config(name, ds=None):
datalad/local/run_procedure.py:def _get_procedure_implementation(name='*', ds=None):

and should be named more inline with other function names (although yeah -- already inconsistent above IMHO) and a little more descriptive :

not also I swapped the order of args to be more inline with other 2 functions and renamed arg so body should change too

Suggested change
def _procedures_exists(ds, cfg_proc):
# Check if specified cfg_proc(s) can be discovered, storing
# the results so they can be used when the time comes to run
# the procedure. If a procedure cannot be found, raise an
# error to prevent creating the dataset.
def _get_proc_configs(names, ds):
"""Get specifications for discovered procedures.
If not a single procedure found, raises `ValueError`
"""

@bpinsard
Copy link
Contributor Author

bpinsard commented Mar 7, 2024

on the right under Paricipants - do you see smth like

? didn't look where generally that is configured in profile
not it isnt' there.
image

@yarikoptic
Copy link
Member

interesting -- you are special @bpinsard ! ;-)

ok, nevermind that -- please address my request for harmonization of naming of the helper and address tests fails and I will take care about adding changelog entry later.

@bpinsard
Copy link
Contributor Author

bpinsard commented Mar 7, 2024

https://github.com/orgs/community/discussions/5634 because the fork is in an org not an individual...

@bpinsard bpinsard changed the base branch from master to maint March 19, 2024 19:09
@bpinsard bpinsard changed the base branch from maint to master March 19, 2024 19:10
Copy link

codeclimate bot commented Mar 19, 2024

Code Climate has analyzed commit a410a0c and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@bpinsard bpinsard requested a review from yarikoptic April 9, 2024 14:51
@yarikoptic
Copy link
Member

tests aren't happy

FAILED ../datalad/distribution/tests/test_install.py::test_install_withcfg - TypeError: __call__() takes from 0 to 1 positional arguments but 2 positional arguments (and 1 keyword-only argument) were given
FAILED ../datalad/distribution/tests/test_install.py::test_recursive_install_withcfg - TypeError: __call__() takes from 0 to 1 positional arguments but 2 positional arguments (and 2 keyword-only arguments) were given

and may be more -- didn't look in detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants