Open Erotemic opened 1 month ago
Attention: Patch coverage is 58.62069%
with 12 lines
in your changes missing coverage. Please review.
Project coverage is 99.81%. Comparing base (
2f3cce5
) to head (1ee309c
).
Files | Patch % | Lines |
---|---|---|
jsonargparse/_signatures.py | 20.00% | 12 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@Erotemic thank you for the pull request. Scriptconfig support like proposed here most likely won't happen. But I will have this in mind to add some public interface that would allow you to do this. Note that this is not a simple topic. New features should behave well with all other existing features and not prevent or make more difficult other potential new features. For example what is done here for aliases could conflict with #301. Please just be patient since this will take time.
For the time being we can use this pull request to test that your integration works with the latest changes.
Thanks for taking the time to look at / review this. Avoiding integration with a particular library makes a lot of sense, but I'm glad you're open to some public API to make something like this possible.
My thought is that ParamData
is the obvious thing to expose to the user. If there was something like:
class MyClass:
__jsonargparse_hook__ = ...
import jsonargparse
parser = jsonargparse.ArgumentParser()
parser.add_class_arguments(MyClass, nested_key='my_class', fail_untyped=False, sub_configs=True)
Where __jsonargparse_hook__
(or a better name) was a callback that users could use to return a custom list of ParamData
objects, then that would allow me to write something specific for scriptconfig to integrate with jsonargparse without jsonargparse having direct knowledge of specific consumer packages. Perhaps the signature looks like:
def __jsonargparse_hook__(params: List[ParamData]) -> List[ParamData]:
"""
Input is the initial list of default parameters that would be added. Modify / add / remove these
and return the modified list to control details of which arguments are exposed to jsonargparse.
"""
However, ParamData
may need to be modified (or given an initial restricted public form that is added to as the default develops). I think the first step is to add support for aliases. It looks like I made a PR to add alias last year that I forgot about: https://github.com/omni-us/jsonargparse/pull/255 but it is outdated and it looks like things have changed.
I don't have as much time to work on FOSS code in the next few months, but jsonargparse is a critical enough part of my workflow that I can prioritize it, but I want to have a solid plan before I commit to anything.
Let me know what you think.
Thank you for the proposal. Though, I don't like it much. I see several issues with it.
lightning
or user implemented module classes. The idea is, it is better that all the logic of a CLI be implemented in a single place, and not requiring a bunch of changes throughout a code base. That is, I don't like this __jsonargparse_hook__
in classes. I know that scriptconfig works like this. But that is no reason to do it similar in jsonargparse.In my previous comment I was asking for patience because right now I am not sure how the integration API would be. And it will take some time for this, since I am focused on other stuff right now. But I will propose something here when ready.
What does this PR do?
This PR is an initial attempt at https://github.com/omni-us/jsonargparse/issues/244
This allows a user to define a scriptconfig object that gives them fine-grained control over the arguments and metadata on the CLI, without requiring the variables to be typed more than once. In other words, the signature of a function is maintained in a class, and it is assumed that the keyword arguments given to that class will be used to create an instance of the scriptconfig object.
Here is what the MWE looks like:
You'll note that it looks similar to dataclass / pydantic / attrs object. However, a major difference is that it doesn't rely on type annotations to store metadata. It uses a special "Value" class, which can be augmented with things like help, aliases, and other information useful to building both a CLI and a function / class signature.
The main scriptconfig page is here for more information: https://gitlab.kitware.com/utils/scriptconfig
I've been using a monkey-patched version of jsonargparse that allows me to work with scriptconfig objects for over a year now, and in late 2023 there was some change to jsonargparse that broke me. This force me to pin jsonargparse and pytorch_lightning to older versions, and I've finally had time to look into it. However, I'd really really really like is there was something codified into jsonargparse that would either directly support scriptconfig or expose some way for users to do custom things when running
add_class_arguments
based on a property in the class itself (which will let it integrate with lightning much easier). I don't particularly care if scriptconfig itself is supported, what I need is something that won't get deprecated or refactored that I can hook into.So far this PR just adds basic support for scriptconfig itself. This involves some extra logic in
_add_scriptconfig_arguments
, and in order to support the alias feature of scriptconfig, I updatedParamData
so it is now equipped with ashort_aliases
and along_aliases
attribute. I've also added a method to it so it can construct the args that will ultimately be passed to argparse. This makes modifications to_add_signature_parameter
a bit cleaner, and also sets the stage to allow other argument sources to specify aliases.Before submitting
WRT to this checklist, I don't want to invest too much time into this before having a discussion with the authors.
I'm looking for feedback on the level of support the maintainers would be willing to provide for something like this. I think the simple thing to do would be to "add scriptconfig support" and be done with it, in which case I could clean up the existing code. The alternative is to allow allow classes to have some attribute (e.g.
__customize_signature__
) that lets the user return a list of additionalParamData
objects that should be recognized by the CLI. However, that involves makingParamData
a public class, so that has its own set of tradeoffs.