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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a feature flag system #2194

Closed
PrettyWood opened this issue Dec 10, 2020 · 11 comments
Closed

Add a feature flag system #2194

PrettyWood opened this issue Dec 10, 2020 · 11 comments
Labels

Comments

@PrettyWood
Copy link
Member

PrettyWood commented Dec 10, 2020

Hi everyone!

Holidays are coming soon 馃巺 and I'm planning on working more on pydantic (currently it's mostly on my very short nights 馃槃)
For example I would like to work on the StrictUnion or the @computed_field and try to close a bunch of issues.
But while working on a very easy to fix issue like this one, I stopped thinking:
"Wait Eric, this is a breaking change. Is it that big of a deal? I don't know! 馃"

Furthermore there are some issues where people want/expect a different global behaviour that could be easily done already and handled by a feature flag (for example all fields without default value are required, even the Optional ones, switching from re.match to re.search for the regex in fields, having a strict mode...)

And this is why I reckon we could benefit from a feature flag system
This would allow us to be less reluctant to make changes and start working for some features of v2 for example
And start adding warning messages like "careful this will be removed in v2...".
Seeing how pydantic is getting popular and how busy @samuelcolvin is, I think we really need to prepare the ground in order to work without stress on the new big features with possible breaking changes

I don't know yet how we would set those feature flags. It could be env variables, config module or/and file or anything else...feedback more than welcome! Here is a very basic example of what could be done.

@samuelcolvin @tiangolo @StephenBrown2 @dmontagu, anyone else?

@NicholasTing
Copy link

Yeah sounds good, would love to be involved in developing this.

@StephenBrown2
Copy link
Contributor

StephenBrown2 commented Dec 11, 2020

Possibly, though first off this issue sounds like a perfect candidate for GitHub's new discussions feature.

I'm not sure how that would be implemented, though I don't think an env var is the best way to go, and pydantic hasn't historically used a conf file. I believe setting a Config var in BaseModel/Settings classes would be the best, since that's already the primary interface, and would allow for comparison between features, and would be explicit, thus "method of least surprise".

@PrettyWood
Copy link
Member Author

PrettyWood commented Dec 11, 2020

Unfortunately we don't have the new discussions feature but I completely agree. Maybe @samuelcolvin can activate it in the repo settings.
As for the implementation I'm not convinced by env variables either don't worry 馃槈 I just want to be sure of the why before thinking too much of the how.
I just don't want to keep saying "create your own custom Model with this" as it annoys me and doesn't help us in the long run and doesn't tackle all the cases. So I would like to make the difference between the Config of a model (which should be used only to customize the way one BaseModel works) and global behaviour changes.

@StephenBrown2
Copy link
Contributor

Yeah, my thought was limiting it to per-model config would be better, but that would be a pain if one wants to enact global changes for all of pydantic.

Perhaps introducing a PydanticConfig singleton class that could be instantiated and models would read from.

But on to the why: I think that it could also be handled with alpha releases (2.0.0a0, 2.0.0a1, etc), with clear documentation on things that have changed AND a request for feedback in case things break too badly, or maybe including the feature flag thing in the alpha, similar to how pip changed the resolver algorithm, but hid it behind --use-feature=2020-resolver for a bit until the kinks had been worked out.

On further thought, an alpha release seems better since breaking changes wouldn't necessarily keep the old behavior around, so some things might be an all-or-nothing approach.

@PrettyWood
Copy link
Member Author

PrettyWood commented Dec 12, 2020

I think talking about alpha releases of v2 is way too early. For me v2 will go even further by rewriting some logic (e.g. the validation workflow possibly, more strictness by default...).
But we could already implement easy stuff right now.

@layday
Copy link
Contributor

layday commented Dec 13, 2020

Would you want to alter the behaviour of all Pydantic models in your dependency tree?

Is this not something model config could accomodate? You could add a new setting which emits a deprecation warning when it's set to False (the default in the current major release). If users would like to opt into the new behaviour for imported libraries, they'd potentially be able to subclass the library models to extend the config.

@PrettyWood
Copy link
Member Author

PrettyWood commented Dec 17, 2020

@layday Would you want to alter the behaviour of all Pydantic models in your dependency tree?

Yes. The "create your own subclass" feels a bit annoying and hacky to really promote new default behaviours
We can also suggest people to do

from pydantic import BaseConfig

BaseConfig.my_disabled_option_by_default = True

to change globally the behaviour of pydantic models instead of defining their own custom BaseModel. At least this way it's obvious and doesn't require any new "GlobalConfig".
I updated #2211 with this for now and added some documentation. It is probably the easiest way.
I'll be happy to change if another way is decided

@PrettyWood
Copy link
Member Author

Related issue: #2003

@samuelcolvin
Copy link
Member

I've just seen this issue, sorry for the slow reply.

I've enabled discussions, I'll change the issue template to point to them for questions and suggest feature requests should seek approval in discussions before becoming issues.


I've thought about this quite a lot and I'm overall opposed to feature flags for pydantic unless absolutely necessary.

I think this is arguably a case of tragedy of the commons - the more complex features and confusing configuration settings we add, the harder pydantic becomes to use for everyone.

My point of view is described perfectly on this prettier issue: prettier/prettier#40

In particular the following line resonates with me:

The requests for configuration are vocal. The requests to stay simple are often silent.

(I came to that issue when researching how to setup prettier to add a space before the opening { in js function definitions (of all weird esoteric desires). After I read that issue, I stopped worrying about my esoteric whims and used the defaults)


On the specific idea of global singleton that modifies all pydantic behaviour, I think this is a bad idea for the following reasons:

  • changing behaviour in one module by importing another is often confusing: imagine someone no longer uses an imported object, they run their code and all is well, they remove the import line and suddenly their codes breaks - this is very confusing, this is what would happen if the code in the main module (or another imported module!) relies on a global configuration which is defined in the module which is no longer imported
  • imagine you use some package which in turn uses pydantic and assumes a particular configuration, now you modify your global config and it breaks obscure behaviour inside a package (you don't know which package, you don' know the problem is in a package, you don't even know you're using packages that use pydantic, you haven't heard of pydantic). 馃挘 hours wasted.

Global configuration belongs to frameworks, not libraries. pydantic is a library, not a framework.


I think the current Config approach is good, custom base classes covers all common cases. They're not magic and require minimal code to use them throughout an application, they are also explicit and opt-in.

The route problem is that progress of pydantic has slowed down somewhat. The solutions for this are:

  • me doing some more work on it
  • me responding to PRs more quickly so I don't end up having to fix them myself because the creator got bored of waiting for me and abandoned the PR
  • using discussions to limit the number of new issues and particularly new PRs for new features which few people want

The reason this has come up is that I have 50 PRs to review whenever I look at pydantic which means I'm less inclined than I might otherwise be to work on pydantic (no one enjoys reviewing PRs as much as writing code). Even when I do work on pydantic I have two or three days of work reviewing PRs before I can work on any of the big behaviour changes people want. I never thought I would say this but there are too many people creating too many PRs!

@PrettyWood
Copy link
Member Author

PrettyWood commented Feb 12, 2021

Thanks a lot @samuelcolvin for the thoughtful and detailed explanation.
This is the kind of moments I cherish when I feel I truly learnt something important or got my eyes open.
I really liked the

The requests for configuration are vocal. The requests to stay simple are often silent.

as well. It will definitely stay up there in my head!
This is actually why I started to fall in love with pydantic in the first place : easy to pick up and use.

I probably tried too hard - and I'm sorry for that I'll stop opening so many PRs - to try to close issues without impacting the library or taking a step back from time to time. With the benefit of hindsight it makes perfect sense.
Anyway thanks again! Glad you answered and sorry for the bother

@samuelcolvin
Copy link
Member

Sorry @PrettyWood, you've misinterpreted what I said (maybe my mistake). Your PRs are not the problem at all, in fact they help because they implement bigger changes and take little time to review. Please keep creating PRs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants