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

Fixing issue in ResolveBackendConfigs #5505

Merged
merged 5 commits into from
Mar 16, 2023

Conversation

oandreeva-nv
Copy link
Contributor

@oandreeva-nv oandreeva-nv commented Mar 15, 2023

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)

         specific_config = {"a":4}
          default_config = {"b":5,"c":6,"d":7}

"c" and "d" are not lost.

And in the following situation
(example)

         specific_config = {"x":4, "y":5, "z":9}
          default_config = {"b":5,"c":6,"d":7}

"y" and "z" are not lost.

GuanLuo
GuanLuo previously approved these changes Mar 15, 2023
tanmayv25
tanmayv25 previously approved these changes Mar 15, 2023
@oandreeva-nv oandreeva-nv dismissed stale reviews from tanmayv25 and GuanLuo via a86eaa3 March 15, 2023 23:05
@oandreeva-nv oandreeva-nv force-pushed the oandreeva_backend_config_issue_5277 branch from a86eaa3 to dce558d Compare March 15, 2023 23:07
@oandreeva-nv
Copy link
Contributor Author

@GuanLuo Let me know if new tests are ok, waiting for CI to run.

@GuanLuo
Copy link
Contributor

GuanLuo commented Mar 15, 2023

Why the test needs to be changed? Shouldn't the core simplification result in the same behavior?

@oandreeva-nv
Copy link
Contributor Author

@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:

Part 1:
if (global_setting < specific_setting) -> final_config.push_back(global_setting) ; increase global_config index
else if (specific_setting > global_setting) -> final_config.push_back(specific_setting); increase specific_config index
else ->   final_config.push_back(specific_setting) increase both
Part 2:
finally if global_config or specific_config still have entries left, add these entries to final_config 

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.

GuanLuo
GuanLuo previously approved these changes Mar 16, 2023
Copy link
Contributor

@GuanLuo GuanLuo left a 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

@oandreeva-nv oandreeva-nv merged commit 4fdd394 into main Mar 16, 2023
@oandreeva-nv oandreeva-nv deleted the oandreeva_backend_config_issue_5277 branch March 16, 2023 21:57
@dyastremsky
Copy link
Contributor

Core PR associated with this one (for posterity): triton-inference-server/core#175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants