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

Migrate configuration command into datalad proper #5489

Closed
mih opened this issue Mar 12, 2021 · 4 comments · Fixed by #6306
Closed

Migrate configuration command into datalad proper #5489

mih opened this issue Mar 12, 2021 · 4 comments · Fixed by #6306
Labels
enhancement UX user experience
Milestone

Comments

@mih
Copy link
Member

mih commented Mar 12, 2021

It is presently provided by an extension and has to be installed separately. However, I find it rather handy, and consider it a candidate for inclusion into core.

https://github.com/mih/datalad-mihextras

https://datalad-mihextras.readthedocs.io/en/latest/generated/datalad.api.x_configuration.html

It was briefly already available, but was removed #5370

@mih mih added enhancement UX user experience labels Mar 12, 2021
@mih mih added this to the 0.15.0 milestone Mar 12, 2021
@mih mih added this to Needs triage in Tackle user experience issues May 20, 2021
@mih mih removed this from the 0.15.0 milestone Sep 7, 2021
@mih
Copy link
Member Author

mih commented Sep 7, 2021

Opinions have not changed. Taking it off the 0.15 milestone.

@mih mih added this to the 0.16.0 milestone Sep 8, 2021
@mih
Copy link
Member Author

mih commented Sep 8, 2021

In order to facilitate a the discussion for the 0.16 release inclusion. Here is a summary of the state.

Pros for inclusion

  • there is no other command line interface to query/modify configuration (apart from git-config)
  • there is no other interface to obtain annotated information on which common configuration items exist
  • there is no other command line accessible documentation for rules of precedence, or env variable naming for config overrides
  • there are no other means than custom code for recursive query/modification of configuration settings
  • ConfigManager requires callers to set reload=False manually when applying multiple modifications

Cons for inclusion

  1. The name is configuration not config
  2. The scope parameter for configuration is not called where
  3. The scope parameter value branch is not called dataset
  4. Modification of system-level settings is not supported by configuration

@mih's rational underlying the cons

re 1. This is done, because the object oriented interface of ConfigManager cannot be mapped onto a joint Python/command line API, hence configuration cannot be the command line sibling of ConfigManager

re 2. where is a parameter of all ConfigManager methods that modify settings. In order to query from different "where" one cannot use ConfigManager.get(), but has to use ConfigManager.get_from_source(), which does not have a where parameter, only source. In contrast, the scope parameter in configuration uniformly applies to both get and set operations.

re 3. As argued in #5363 the name dataset is inaccurate.

re 4. ConfigManager does not distinguish global from system, and configuration is a thin frontend for it. Supporting a system scope (that was not needed for any DataLad functionality so far) would require a fairly substantial change to ConfigManager, or a partial replacement within configuration.

@mih's conclusion from the discussion

The inclusion of configuration comes with the declared precondition of a cleanup of the ConfigManager API to rectify suboptimal implementations of the past. Given that this API is deeply engrained into a lot of code, the associated transition needs to be done with care and a long deprecation cycle. @mih considers such an API cleanup a laudable goal, but cannot get behind judging the importance of that higher than the listed pros for inclusion of configuration. The alternatively requested renaming of parameters and their values would only establish superficial alignment of the APIs, and additionally contaminate the API components with the problems of the past.

@yarikoptic
Copy link
Member

yarikoptic commented Sep 8, 2021

The alternatively requested renaming of parameters and their values would only establish superficial alignment of the APIs

Re 2 and 3 which are IMHO a singular aspect. As we have discussed on the call, renaming could as well be done at ConfigManager level (where -> scope; dataset -> branch) through a deprecation cycle (e.g. add for 0.15.0 and deprecate/remove old style in 0.16.0 or later ;) ), thus gently transitioning/preparing users to use configuration options and not requiring them to remember what option=value to use with what API endpoint. IMHO it would not be a "superficial alignment" since AFAIK they refer to exactly the same thing, and I do like scope and branch better (even surprised I did not complain/suggest it whenever where="dataset" was introduced in #922).

And FWIW I don't care about 4 (system-level) and also do not see how to avoid 1 in a reasonable effort.

@mih
Copy link
Member Author

mih commented Sep 8, 2021

Ok, so someone should do this ConfigManager PR within the next 6 month.
I will follow up with the actually visible change right after that. Together we can solve this in just little more than one year, while avoiding any misalignments ;-)

This gives me plenty of time to compile the list of issues asking for consistent configuration settings to be established in a dataset hierarchy.

@mih mih moved this from Needs triage to High(er) priority in Tackle user experience issues Oct 6, 2021
mih added a commit to mih/datalad that referenced this issue Dec 13, 2021
mih added a commit to mih/datalad that referenced this issue Dec 13, 2021
mih added a commit to mih/datalad that referenced this issue Dec 14, 2021
mih added a commit to mih/datalad that referenced this issue Dec 14, 2021
mih added a commit to mih/datalad that referenced this issue Jan 29, 2022
@mih mih closed this as completed in #6306 Jan 31, 2022
Tackle user experience issues automation moved this from High(er) priority to Closed Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement UX user experience
Development

Successfully merging a pull request may close this issue.

2 participants