-
Notifications
You must be signed in to change notification settings - Fork 897
[UX] Deduplicate config warning messages #8014
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
[UX] Deduplicate config warning messages #8014
Conversation
Fixes skypilot-org#7874 The warning about misaligned client/server config was appearing twice per command execution. This adds a per-run_id cache to deduplicate warnings within a single command while still showing them for new commands. Tested: - Manually verified with 'sky show-gpus --infra k8s' - Warning now appears once instead of twice - Subsequent commands still show the warning (different run_id)
|
Thanks @atoniolo76! Could you help fix the CI failure and add a unit test for your implementation? |
…clearing when it exceeds 1000 warning entries
|
@Michaelvll Good to go! |
SeungjinYang
left a comment
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.
Thanks @atoniolo76! Couple of minor comments, and the PR should be ready to go.
sky/skypilot_config.py
Outdated
| 'and will be ignored. Remove these keys to disable this warning. ' | ||
| 'If you want to specify it, please modify it on server side or ' | ||
| 'contact your administrator.') | ||
| run_id = os.environ.get(usage_constants.USAGE_RUN_ID_ENV_VAR) |
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.
What do you think about using common_utils.get_usage_run_id here instead?
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.
Env var is always set but I agree this is cleaner
… not reset between runs
Fixes skypilot-org#7874 The warning about misaligned client/server config was appearing twice per command execution. This adds a per-run_id cache to deduplicate warnings within a single command while still showing them for new commands. Tested: - Manually verified with 'sky show-gpus --infra k8s' - Warning now appears once instead of twice - Subsequent commands still show the warning (different run_id)
…clearing when it exceeds 1000 warning entries
… not reset between runs
…niolo76/skypilot into fix-duplicate-config-warning
2b76478 to
55fc731
Compare
|
@SeungjinYang I have implemented your suggestions and updated the associated unit test. Ready to merge? |
There's something that I'm hesitant about with merging this PR. Cache in this case is in-memory, so its state are not shared between processes. In cases such as The part that perplexed me was the fact that the unit test clearly works, and your local test clearly works. Given the explanation above, it makes sense why unit test introduced may not be testing the case where two API request lands on two different processes. As for local test, I believe local API uses a single server process by default, so a locally spun up API server would not run into the issue described above. A remotely deployed team API server, on the other hand, can and do have multiple server processes, and I think the change proposed may not function reliably in such cases. I believe to guard against this scenario, the cache would have to be set up in a way that multiple processes can share the cache state. |
|
@SeungjinYang Didn't think about the remote scenario, moved to a shared solution. Made a new pr too, closing this one |
Fixes #7874
The warning about misaligned client/server config was appearing twice per command execution. This adds a per-run_id cache to deduplicate warnings within a single command while still showing them for new commands.
Tested:
Tested (run the relevant ones):
bash format.shsky show-gpus --infra k8s/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)