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

chore(hybrid-cloud): Resubmits Pydantic v2.7 upgrade #75311

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

GabeVillalobos
Copy link
Member

Mulligan of PR #74770

This PR upgrades pydantic and updates the model configs we use for RPC methods to conform with the new version.

This PR does not address deprecation warnings, which will be handled in a series of follow-up commits to both Sentry and GetSentry.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 30, 2024
Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.21%. Comparing base (2525223) to head (f3fa688).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #75311      +/-   ##
==========================================
+ Coverage   78.18%   78.21%   +0.02%     
==========================================
  Files        6775     6775              
  Lines      302017   302019       +2     
  Branches    51963    51963              
==========================================
+ Hits       236134   236224      +90     
+ Misses      59513    59440      -73     
+ Partials     6370     6355      -15     
Files Coverage Δ
src/sentry/autofix/utils.py 93.75% <100.00%> (ø)
src/sentry/hybridcloud/rpc/__init__.py 82.20% <100.00%> (-0.15%) ⬇️
src/sentry/hybridcloud/rpc/sig.py 90.78% <100.00%> (+0.24%) ⬆️
src/sentry/types/region.py 91.41% <100.00%> (+0.05%) ⬆️
src/sentry/users/services/user/serial.py 93.33% <ø> (ø)

... and 31 files with indirect coverage changes

@GabeVillalobos GabeVillalobos marked this pull request as ready for review July 31, 2024 17:30
@GabeVillalobos GabeVillalobos requested review from a team as code owners July 31, 2024 17:30
@asottile-sentry
Copy link
Member

Mulligan of PR #74770

any context on what's changed since last attempt? and why the last one failed?

@asottile-sentry
Copy link
Member

the api diff also appears to be broken 🤔

@GabeVillalobos
Copy link
Member Author

@asottile-sentry For some reason, a pod was OOMing in S4S after the upgrade. It's unclear why this was occurring as this particular pod shouldn't have been affected, but we're planning to try scaling the pod up a bit to see if this alleviates the issue.

As for the API diff errors, it looks like pydantic's generated JSON schema handles nullable fields differently in V2 than it did in V1 Pydantic docs on the changes

AppService.create_internal_integration_for_channel_request Schema

Old Schema:

"AppService__create_internal_integration_for_channel_request__ParameterModel": {
  "properties": {
    "organization_id": {"type": "integer", "title": "Organization Id"},
    "integration_name": {"type": "string", "title": "Integration Name"},
    "integration_scopes": {
      "items": {"type": "string"},
      "type": "array",
      "title": "Integration Scopes"
    },
    "integration_creator_id": {
      "type": "integer",
      "title": "Integration Creator Id"
    },
    "metadata": {"type": "object", "title": "Metadata"}
  },

New Schema

"AppService__create_internal_integration_for_channel_request__ParameterModel": {
  "properties": {
    "organization_id": {"type": "integer", "title": "Organization Id"},
    "integration_name": {"type": "string", "title": "Integration Name"},
    "integration_scopes": {
      "items": {"type": "string"},
      "type": "array",
      "title": "Integration Scopes"
    },
    "integration_creator_id": {
      "type": "integer",
      "title": "Integration Creator Id"
    },
    "metadata": {
      "anyOf": [{"type": "object"}, {"type": "null"}],
      "title": "Metadata"
    }
  },
  "type": "object",
  "required": [
    "organization_id",
    "integration_name",
    "integration_scopes",
    "integration_creator_id"
  ],
  "title": "AppService__create_internal_integration_for_channel_request__ParameterModel"
},

The new schema appears to be more accurate given the method definition:

def create_internal_integration_for_channel_request(
    self,
    *,
    organization_id: int,
    integration_name: str,
    integration_scopes: list[str],
    integration_creator_id: int,
    metadata: dict[str, Any] | None = None,
) -> RpcSentryAppInstallation:
    ...

@asottile-sentry
Copy link
Member

cool just checking -- more memory makes sense to me given pydantic 2's move to a rust extension over cython so I wouldn't be surprised if a few workers need slight bumps (especially if they're running particularly hot)

Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

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

@GabeVillalobos GabeVillalobos merged commit 0d8acb9 into master Jul 31, 2024
52 of 53 checks passed
@GabeVillalobos GabeVillalobos deleted the gv/resubmit-pydantic-upgrade branch July 31, 2024 19:11
corps added a commit that referenced this pull request Aug 1, 2024
corps added a commit that referenced this pull request Aug 1, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants