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

Allow empty tunable values to represent the defaults #868

Conversation

bpkroth
Copy link
Contributor

@bpkroth bpkroth commented Oct 4, 2024

Pull Request

Title

Allows an empty dictionary (object) to represent the default tunable values for tunable params.


Description

This should make it easier to maintain configs that loop over a chosen set of tunable values, where some of the tunable values are the default values without needing to copy/paste those values from the tunable params definitions.


Type of Change

  • ✨ New feature

Testing

Extends existing testing infrastructure to check that this works.


@bpkroth bpkroth marked this pull request as ready for review October 4, 2024 16:40
@bpkroth bpkroth requested a review from a team as a code owner October 4, 2024 16:40
@bpkroth bpkroth mentioned this pull request Oct 4, 2024
@bpkroth
Copy link
Contributor Author

bpkroth commented Oct 4, 2024

As noted in the linked discussion, an alternative could be to allow null to represent the defaults, but this seemed reasonably clean.

Prior to this {} would basically be a no-op (i.e., doesn't change any of the values of the current config), which I guess also makes sense, but is also probably also confusing.

@motus
Copy link
Member

motus commented Oct 4, 2024

Looks good! Maybe, all one more unit test with some tunable values omitted to check that they are set to default and others are not?

@bpkroth
Copy link
Contributor Author

bpkroth commented Oct 4, 2024

Looks good! Maybe, all one more unit test with some tunable values omitted to check that they are set to default and others are not?

I think that's handled already in the test I added. I made it a little bit more explicit just now as well.

@bpkroth bpkroth enabled auto-merge (squash) October 4, 2024 21:35
@bpkroth bpkroth merged commit dbaccea into microsoft:main Oct 4, 2024
12 of 13 checks passed
@bpkroth bpkroth deleted the allow-empty-tunable-values-dict-to-represent-defaults branch October 4, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants