-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fixing issue in ResolveBackendConfigs #5505
Conversation
a86eaa3
to
dce558d
Compare
@GuanLuo Let me know if new tests are ok, waiting for CI to run. |
Why the test needs to be changed? Shouldn't the core simplification result in the same behavior? |
@GuanLuo The former logic involved sorting. Global configs and Specific configs were first sorted lexicographically. Then we iterated over global configs and specific configs and populated final config with settings:
The initial issue was in the last part, when not all entries were added to final config in the end. Thus tests were designed so that to test edge cases related to iteration over 2 sorted arrays: 1)if more than 1 entry is left in global config, Part 2 adds all of them to final config, and 2) vice versa. Since updated logic does not depend on sorting any longer and we iterate first over the whole global config (also default config), then over the whole specific config, the former tests did not make sense. New tests make sure, that specific configs are added to final configs in full, and any default parameters that need to be overriden, are overriden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update copyright year at the top
Core PR associated with this one (for posterity): triton-inference-server/core#175 |
This PR adds tests to check edge cases for backend configuration resolution.
Current logic for backend config resolution sorts default configs and specific configs lexicographically. Then it iterates through them and prioritizes backend specific config over the default one.
We would like to make sure that in the following situation (example)
"c" and "d" are not lost.
And in the following situation
(example)
"y" and "z" are not lost.