-
Notifications
You must be signed in to change notification settings - Fork 86
[RAPTOR-12427] Less verbose 422 when reporting monitoring data from chat completions #1495
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
Conversation
The Needs Review labels were added based on the following file changes. Team @datarobot/core-modeling (#predictive-ai) was assigned because of changes in files:custom_model_runner/datarobot_drum/drum/language_predictors/base_language_predictor.py Team @datarobot/genai-systems (#custom-models) was assigned because of changes in files:custom_model_runner/datarobot_drum/drum/language_predictors/base_language_predictor.py tests/unit/datarobot_drum/drum/language_predictors/test_base_language_predictor.py If you think that there are some issues with ownership, please discuss with C&A domain at #sdtk slack channel and create PR to update DRCODEOWNERS\CODEOWNERS file. |
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.
LGTM for PredAI, but consider using builtin caplog
instead of having to patch the logger yourself.
mock_warning.assert_called_once() | ||
args, _ = mock_warning.call_args | ||
assert exception_string in args[0] |
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.
This works fine, but did you know about the magic pytest builtin fixture caplog
? This fixture is always available and basically does what you want here, I believe.
Pytest docs about it: https://docs.pytest.org/en/stable/how-to/logging.html
An example of use in the monorepo: https://github.com/datarobot/DataRobot/blob/06dc3995c35c2bcd52c476eac1257dad0faa6b8a/MLDev_Tests/autopilot/tests/backend/integration/stages/test_bias_mitigation_stage.py#L351-L358
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.
Fixed it
mock_warning.assert_called_once() | ||
args, _ = mock_warning.call_args | ||
assert exception_string in args[0] | ||
assert True |
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.
Nit: this clearly does nothing.
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.
Fixed it
…hat completions When Text Gen deployment is created, the feature drift and predictions data collections are off by default. When /chat/completions request is received DRUM will report monitoring data using API Spooler and will fail with 422 - creating big stack trace. If, by default both flags are off, particularly for this use case of DRUM + chat completions, DRUM shouldn’t be this verbose. This patch will put the warning - but not the stack trace
6499537
to
17bb8f9
Compare
Merged PR by @amarmudrankit based on Auto Merge Label |
Summary
When Text Gen deployment is created, the feature drift and predictions data collections are off by default. When /chat/completions request is received DRUM will report monitoring data using API Spooler and will fail with 422 - creating big stack trace. If, by default both flags are off, particularly for this use case of DRUM + chat completions, DRUM shouldn’t be this verbose.
This patch will put the warning - but not the stack trace
Manually tested it to ensure warning is printed but not the stack trace.
This repository is public. Do not put here any private DataRobot or customer's data: code, datasets, model artifacts, .etc.