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

Remove remnants of langchain and update to pydantic 2 #569

Merged
merged 2 commits into from
Jul 31, 2023
Merged

Remove remnants of langchain and update to pydantic 2 #569

merged 2 commits into from
Jul 31, 2023

Conversation

rec
Copy link
Contributor

@rec rec commented Jul 30, 2023

Description

#565 changed requirements.txt but did not regenerate the dependencies!

This pull request fixes that, and then updates to the long-awaited pydantic 2 which we are finally able to install.

(We aren't using any Pydantic 2 features yet, and we should probably let it sit for a bit before we do in case we suddenly have to backgrade.)

@rec rec requested a review from blythed July 30, 2023 10:35
@rec
Copy link
Contributor Author

rec commented Jul 31, 2023

Interestingly enough, this works locally in a clean repo but fails upstream with an error that 60 seconds' work here did not repro.

This isn't a priority but I'd like to keep this pull request open for a little longer so I can experimen t later in the week.

Copy link
Collaborator

@blythed blythed left a comment

Choose a reason for hiding this comment

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

Ok.

@nenb
Copy link
Contributor

nenb commented Jul 31, 2023

@blythed I don't think we should merge this even if we fix the issue with the tests.

The reason is that by pinning pydantic to be greater than 2.X we become incompatible with any package that has pydantic pinned to be less than 2.X. This could be quite a common scenario for several reasons (eg poetry will by default pin pydantic to be less than 2.X if you install pydantic 1.X).

I don't consider pydantic 2.X to be absolutely essential to our use-case at the moment (although I admit it's very useful). As such, I don't think we should create this kind of conflict (and frustrated users) for now.

@rec Let me know if you think pydantic 2.X is essential, I don't have visibility over everything.

@rec
Copy link
Contributor Author

rec commented Jul 31, 2023

The reason is that by pinning pydantic to be greater than 2.X we become incompatible with any package that has pydantic pinned to be less than 2.X.

Yes, I understand that entirely of course.

But so far we do not have such a conflict, and the only way we will find out if someone else has this conflict is by putting it out there.

If someone contacts us to complain, this is a good thing, particularly if we can quickly revert (because we aren't using the new features). "look how responsive they are!" :-D


The use case for pydantic itself is strong, but unfortunately, without custom serialization, a 2.x only feature, we were unable to make it work, which is why we put it aside. (I'd be very willing to through the long and ;-) disheartening chain of reasoning there.)

  1. Currently, we have no schema for the REST API. And if someone sends in something of the wrong type or with extra fields or fields of the wrong type, it's almost certain our code will crash and give a 500 error. FastAPI's big advantage is the very clear validation error messages, so that if it finally gets to the code, we know that everything is in the right place.

  2. Without it, we will have to prepare and maintain an OpenAPI spec by hand. Preparing it the first time won't be too bad but updating it each time something tiny changes will not be so much fun.

  3. We also get the very slick "redoc" and "docs" endpoints that are extremely useful for casual experimentation and debugging.


So my plan was to update to 2.x but not use any features in the production code that uses it. I can explore in unit tests, behind a feature flag, to see if this really fixes our issues or not.

At the same time, we can find out if this restriction is a problem for anyone.

Another possibility is not to add the dependency, and I can still write the unit tests behind a feature flag checking the pydantic version.

@rec
Copy link
Contributor Author

rec commented Jul 31, 2023

(Oh, I should add, I'll fix the error here anyway, whether we use this or not, but later...)

@rec
Copy link
Contributor Author

rec commented Jul 31, 2023

So I pushed the fix for pydantic 2.x onto this branch but removed the change pinning pydantic 2.x to another branch so we can push this one ASAP.

@nenb nenb self-requested a review July 31, 2023 16:35
Copy link
Contributor

@nenb nenb left a comment

Choose a reason for hiding this comment

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

That was quick! 🚀

@rec rec merged commit 49a18bd into superduper-io:main Jul 31, 2023
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.

3 participants