Skip to content

Conversation

danielgafni
Copy link

@danielgafni danielgafni commented Sep 19, 2025

For reviewers

  • I used AI and thorougly reviewed every code/docs change

Description of the change

Add a new configuration option: extra_bases. This allows extending the default list (pydantic.BaseModel, pydantic.main.BaseModel) with third-party base pydantic classes.

Relevant resources

Resolve #41, #14, #27

@danielgafni
Copy link
Author

A few things:

  1. I've run ruff on my files and looks like it introduced a few minor formatting changes. Seems like ruff isn't enforced on the repo?
  2. I've tested this code on my dagster-ray lib, it works!
image 3. I couldn't run the tests locally since I'm not NixOS and `pdm` doesn't really work here (I'm using `uv` for all the other projects I'm working on). Hopefully we can just run them in CI.

@danielgafni
Copy link
Author

danielgafni commented Sep 19, 2025

Never mind, figured out you had a non-standard ruff.toml location

@danielgafni danielgafni force-pushed the feat/configurable-bases branch 3 times, most recently from bab7f30 to 7d73e90 Compare September 19, 2025 11:37
@danielgafni danielgafni force-pushed the feat/configurable-bases branch from 7d73e90 to 8bc318c Compare September 19, 2025 11:41
assert "pydantic-field" not in package["Model.a"].labels


def test_extra_bases() -> None:
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if it's a good test since both classes are implemented in the same package. Should we split them to separate packages? or what's the best way to test this?

@pawamoy
Copy link
Member

pawamoy commented Sep 19, 2025

Yeah we provide a make format command to format the code, it takes care of using the right config file (located in config/ruff.toml as you noticed).

About NixOS / PDM, not sure why pdm-backend wouldn't work on this OS 🤷 We're using uv too, but building the project with pdm-backend (which is transparently installed as a Python package, in a temp venv, by uv when building, so it should work on any OS really).

Let's decide on the issue whether the preload_modules solution works for you before I review this code 🙂

@danielgafni
Copy link
Author

danielgafni commented Sep 19, 2025

I guess I was too quick with my judgement: indeed pdm works with a uv-provided Python executable (uv installs standalone executables which work on NixOS since they don't rely on shared libraries). So the main uv role here is just to provide a "good" Python executable on NixOS -- the package management part doesn't really matter.

So, as mentioned in #41, preloading dagster breaks things for me. Not sure if there is an easy work-around but this is already good evidence that this feature might be useful anyway (it just has a smaller scope -> no chance to break or affect anything else).

@sneakers-the-rat
Copy link

Basically the same as #30 except imports done inline rather than pulling out to separate functions

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.

feature: support additional user-provided base classes
3 participants