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

πŸ—οΈ Settings: From pydantic BaseModel to custom class #46

Merged
merged 7 commits into from
Jun 25, 2022

Conversation

falexwolf
Copy link
Member

@falexwolf falexwolf commented Jun 25, 2022

pydantic isn't so great in our case:

  • As during setup, settings are written iteratively and need functionality from previous settings, there is no simple point in time where we can instantiate and validate all settings using pydantic BaseModel.
  • BaseModel does not support editable docstrings on the attribute level, which makes it hard to share documentation. (Edit: It turns out that it's also not so easy with ordinary classes, I thought I had it work before. πŸ€·β€β™‚οΈ)
  • BaseModel does not recognize CloudPath and can't validate it, which is why we had to introduce storage_root_str, defeating almost the whole point of a typed Settings class.

Using a custom dataclass, now we migrated to pickle instead of writing a JSON. Let's see whether we regret it. Pro is that it supports CloudPath and Path, which JSON doesn't. Con is that it's not human-readable and I might not see a way in which it might break.

A big change is also that we are now using setup.settings() instead of setup.settings. The latter always resulted in some quirks in some form. For instance, no auto-complete when using the "module attribute" hack (__getattr__("settings")). It seems cleaner to have a callable who will always read the current state of settings.

An alternative is to require shut down of the kernel after setup has been run, but that seems bad for the UX at least in the current state.

@github-actions
Copy link

github-actions bot commented Jun 25, 2022

@github-actions github-actions bot temporarily deployed to pull request June 25, 2022 14:13 Inactive
@falexwolf falexwolf changed the title πŸ—οΈ Migrate from pydantic BaseSettings to custom … πŸ—οΈ Migrate from pydantic BaseSettings to custom class Jun 25, 2022
@falexwolf falexwolf changed the title πŸ—οΈ Migrate from pydantic BaseSettings to custom class πŸ—οΈ Migrate from pydantic BaseModel to custom class Jun 25, 2022
@falexwolf falexwolf changed the title πŸ—οΈ Migrate from pydantic BaseModel to custom class πŸ—οΈ Settings: From pydantic BaseModel to custom class Jun 25, 2022
@github-actions github-actions bot temporarily deployed to pull request June 25, 2022 15:01 Inactive
@falexwolf falexwolf merged commit 7287d27 into main Jun 25, 2022
@falexwolf falexwolf deleted the settings branch June 25, 2022 15:02
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.

1 participant