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.

Sign up for GitHub

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

Jump to bottom

Changes to be compatible with jsonargparse v4 #92

Closed
wants to merge 2 commits into from
Closed

Changes to be compatible with jsonargparse v4 #92

wants to merge 2 commits into from

Conversation

mauvilsa
Copy link
Contributor

Motivation

I have noticed that this project has as dependency jsonargparse and makes use of the ActionJsonSchema class. Scheduled for tomorrow (Nov 16th 2021) jsonargparse v4.0.0 will be released and it includes a breaking change in the behavior of ActionJsonSchema. The change is that the values parsed with this action will no longer be a nested namespace but a nested dict instead.

Changes proposed

This pull request includes a small change so that there is compatibility with the newer versions of jsonargparse.

Test Plan

I tested by using pplbench examples/example.json with jsonargparse v4.0.0rc1 making sure that it worked just like with previous versions.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • The title of my pull request is a short description of the requested changes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 15, 2021
@jpchen
Copy link

jpchen commented Nov 15, 2021

Thanks for doing this @mauvilsa! Could you also bump the version in setup.py?

@facebook-github-bot
Copy link
Contributor

@jpchen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jpchen
jpchen reviewed Nov 15, 2021
View reviewed changes
pplbench/main.py Outdated Show resolved Hide resolved
@mauvilsa
Copy link
Contributor Author

Thanks for doing this @mauvilsa! Could you also bump the version in setup.py?

You mean bump the version of jsonargparse so that the minimum is 4.0.0? If the minimum is 4.0.0 then the if isinstance(config, dict): is not necessary.

@mauvilsa
Copy link
Contributor Author

Thanks for doing this @mauvilsa! Could you also bump the version in setup.py?

You mean bump the version of jsonargparse so that the minimum is 4.0.0? If the minimum is 4.0.0 then the if isinstance(config, dict): is not necessary.

Done

@mauvilsa
Copy link
Contributor Author

@jpchen the changes you requested were done. Is there anything else to do?

@jpchen
Copy link

jpchen commented Dec 9, 2021

LGTM, will merge when internal tests pass. Thanks @mauvilsa !

@facebook-github-bot
Copy link
Contributor

@jpchen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

horizon-blue pushed a commit to horizon-blue/pplbench that referenced this pull request Jan 26, 2022
@mauvilsa @facebook-github-bot
Changes to be compatible with jsonargparse v4 (facebookresearch#92)
af38963 
Summary:
### Motivation
I have noticed that this project has as dependency jsonargparse and makes use of the `ActionJsonSchema` class. Scheduled for tomorrow (Nov 16th 2021) jsonargparse v4.0.0 will be released and it includes a breaking change in the behavior of `ActionJsonSchema`. The change is that the values parsed with this action will no longer be a nested namespace but a nested dict instead.

### Changes proposed
This pull request includes a small change so that there is compatibility with the newer versions of jsonargparse.

Pull Request resolved: facebookresearch#92

Test Plan:
I tested by using `pplbench examples/example.json` with jsonargparse v4.0.0rc1 making sure that it worked just like with previous versions.

### Types of changes
- [x] Docs change / refactoring / dependency upgrade
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

### Checklist
- [x] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have read the **[CONTRIBUTING](https://github.com/facebookresearch/pplbench/blob/main/CONTRIBUTING.md)** document.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
- [x] The title of my pull request is a short description of the requested changes.

Differential Revision: D32424133

Pulled By: horizon-blue

fbshipit-source-id: 050cb3007bfdf47455cf3437d62d0e7838056211
@horizon-blue
Copy link
Contributor

Sorry for taking a while to get this PR in. We had to update jsonargparse in our internal platform as well and make sure the tests still pass, which is why it's been taking so long. Regardless, thank you for taking the effort to maintain the compatibility of the package :D

@mauvilsa mauvilsa deleted the jsonargparse-4-compatibility branch January 26, 2022 04:57
@agunapal agunapal mentioned this pull request Apr 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpchen jpchen jpchen left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mauvilsa @jpchen @facebook-github-bot @horizon-blue