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

fix(api): Parse all RTP fields strictly to fix flakiness #15187

Merged
merged 4 commits into from
May 16, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented May 15, 2024

Overview

Closes AUTH-401.

Changelog

As described in AUTH-401, there were two problems:

  1. Our Pydantic models sometimes parse run-time parameter values as the wrong types. For example, parsing true into Union[float, bool, str] could return 1.0 instead of True. This is because of Pydantic's type coercion.
  2. Exactly which values get wrongly parsed into exactly which wrong types is unstable; it changes depending on things like import statements in files far away. This is apparently because, although Pydantic parses unions “left to right,” the order of “left to right” can change depending on Python’s caching of generic types.

The solution to both of these problems is to disable Pydantic's type coercion. That solves the first problem for obvious reasons. It solves the second problem because, when parsing something like a Union[float, bool, str], there will no longer be any "ties" that Pydantic needs to break by using Union order.

(In my opinion, this is also a good practice in general, and we should do it for everything going forward.)

For all fields in all run-time parameter models, change int to StrictInt, str to StrictStr, bool to StrictBool, and float to Union[StrictInt, StrictFloat]. That last Union is because numbers like 123, without a decimal point, do not parse into StrictFloat.

Test plan

  • In robot-server, run pipenv run pytest tests/protocols/test_protocols_router.py::test_create_protocol_analyses_with_same_rtp_values on commits c90840d and 65a57e8. It will fail on the first one (before the fix) and succeed on the second one (after the fix).
  • Continued overall testing of run-time parameters? See risk assessment.

Review requests

Are there any fields or models that I missed?

Also see risk assessment.

Risk assessment

Medium?

Changes like this can break the database if we've been accidentally storing stuff wrongly, like if we've been storing "1.23" where we meant to store 1.23 and this PR changes that field from float to StrictFloat.

There's also some risk that the frontend has had bugs where it sends the wrong types, but the server-side type coercion has been masking them. The server will return 422 errors now.

I'll defer to AUTH people for their assessment of how likely we are to have problems like these, and how much manual testing we need to do to be confident.

I have no idea why this is the case, but adding this import changes the result of:

pipenv run python -c 'from robot_server.protocols.analysis_models import AnalysisRequest; print(AnalysisRequest(runTimeParameterValues={"vol": 123, "dry_run": True, "mount": "left"}))'
@SyntaxColoring SyntaxColoring marked this pull request as ready for review May 15, 2024 17:11
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner May 15, 2024 17:11
@sanni-t sanni-t requested a review from ncdiehl11 May 15, 2024 17:20
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing this

@sanni-t
Copy link
Member

sanni-t commented May 15, 2024

@ncdiehl11 verified that there are no app-side changes for this

@SyntaxColoring
Copy link
Contributor Author

SyntaxColoring commented May 15, 2024

Thank you both for the quick reviews and tests! This was an adventure. 🗺️

[Edit] Oh, is it correct for this to merge into edge? Discussing in Slack.

@y3rsh
Copy link
Member

y3rsh commented May 16, 2024

Edge please. Not enough time to test before the release. Especially with possible DB side effects.

I need to add type assertions on all the RTP at runtime in my snapshot test. On my list now.

@SyntaxColoring SyntaxColoring merged commit 1e66d70 into edge May 16, 2024
20 checks passed
@SyntaxColoring SyntaxColoring deleted the fix_flaky_parse branch May 16, 2024 13:32
shlokamin added a commit that referenced this pull request Jun 27, 2024
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.

3 participants