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

Simplify PrefectBaseSettings #15290

Closed
aaazzam opened this issue Sep 9, 2024 · 1 comment · Fixed by #15412
Closed

Simplify PrefectBaseSettings #15290

aaazzam opened this issue Sep 9, 2024 · 1 comment · Fixed by #15412
Labels
enhancement An improvement of an existing feature

Comments

@aaazzam
Copy link
Collaborator

aaazzam commented Sep 9, 2024

Describe the current behavior

Prefect's settings.py is roughly ~150 custom Setting objects which is dynamically collected into a BaseSettings object at runtime. By dynamically collecting these at runtime, this

  • obfuscates typing and
  • makes it difficult to ergonomically inherit from in derivative frameworks (like ControlFlow).

For instance, it's difficult to change the default value of a single setting given it's current design, but could be trivial if PrefectBaseSetting was a simple static subclass of Pydantic's BaseSetting.

From examining it's original PRs, the current design was taken on:

  • to attach validators to fields themselves, and not as field_validators on the Settings object.
  • to create settings dependent on the value of other settings.

These two outcomes required a sophisticated design two years ago, but are now first-class concepts in Pydantic: AfterValidator and computed_field, respectively.

Describe the proposed behavior

I propose having Prefect's BaseSetting's object be a canonical subclass of Pydantic's BaseSetting:

class PrefectBaseSetting(pydantic_settings.BaseSetting)
    PREFECT_WHATEVER_1 = Field(...)
    ...
    PREFECT_WHATEVER_150 = Field(...)

Example Use

No response

Additional context

No response

@aaazzam aaazzam added the enhancement An improvement of an existing feature label Sep 9, 2024
@cicdw
Copy link
Member

cicdw commented Sep 9, 2024

I'm adding this to our Settings milestone not necessarily as a complete acceptance, but either way I want to have a strong stance on why we did or did not accept this (my instinct is that we'll ultimately accept it, for the record).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants