-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Initial scriptconfig support #532
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@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
Where
However, 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.
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. |
[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Fix test
9039ad5
to
f574e3f
Compare
Quality Gate passedIssues Measures |
Closing as this is not planned to be integrated, although I will continue to maintain this branch as my use-cases require some sort of way to define parameters without relying on the python annotation system or docstrings. |
What does this PR do?
This PR is an initial attempt at #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.