[REF-2127] Loosen requirements #2796
Conversation
|
Would be great if the integration tests could run again. I'll try to reproduce the setup locally, but especially for windows, I do not have access to a machine to test locally. |
|
@mahrz24 If you mark it as "Ready for review" the integration tests should run on every merge |
mahrz24
force-pushed
the
loosen-requirements
branch
from
57ed1c1 to
d82baed
Compare
mahrz24
force-pushed
the
loosen-requirements
branch
from
ac27357 to
743d200
Compare
|
@picklelo I converted and rebased fresh, but it seems it still needs approval. However I think this now addresses all issues and is compatible with a larger range of versions and the latest version of most dependencies. I had to make two changes to the code. The first one is making it compatible with pydantic v2 but using the v1 API still. This way a future v2 migration will be more easy and users of reflex will not yet be forced to use v2 directly. It would be great if the hosting cli tools could also use the same approach and lift the down pinning of pydantic. The second change concerns the handling of uploads. Fastapi moved the exit stack handler so that file are closed before reflex handles them. I haven't added tests that automatically repin with these more loose requirements and I am not sure how you would prefer this exactly to work. |
There was a problem hiding this comment.
i like the change... i think the risk here is that we continue to test with our poetry.lock file, but users might install the package from pip and end up getting untested versions of dependencies.
We need some kind of automation to ensure that our lock file is always up to date, particularly before a release, to ensure that the latest versions of packages are at least tested.
Further, someone might be pinning an older reflex version that worked with the latest versions of the packages at the time it was released, but becomes broken with subsequent releases of dependencies. Unless all of our users are pinning all of their transitives (like they should), we could end up in a situation where a downstream user cannot recreate a working environment without upgrading their reflex version (and potentially hitting deprecations and changes in reflex itself that they don't have time to deal with).
I want to move forward with this, but i think we should wait at least another release and try to bring it in next week.
| # Make a copy of the files as they are closed after the request. | ||
| # This behaviour changed from fastapi 0.103.0 to 0.103.1 as the | ||
| # AsyncExitStack was removed from the request scope and is now | ||
| # part of the routing function which closes this before the | ||
| # event is handled. | ||
| file_copies = [] | ||
| for file in files: | ||
| content_copy = io.BytesIO() | ||
| content_copy.write(await file.read()) | ||
| content_copy.seek(0) | ||
| file_copies.append( | ||
| UploadFile( | ||
| file=content_copy, | ||
| filename=file.filename, | ||
| size=file.size, | ||
| headers=file.headers, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
@picklelo if we're going to do this anyway, maybe we should re-evaluate whether to still let the FastAPI UploadFiles interface leak through the reflex abstraction.
There was a problem hiding this comment.
Agreed, let's look more into this next week. I'll make sure the file uploads continue to work with this method.
|
@masenf Thanks for the feedback. I agree with with your assessment. I don't know how your release process is right now. If it is manual, simply creating a new PR with an updated lock file before each release would probably enough, as long as you keep up a quite frequent release cycle like it is now. The problem of older versions at some point not working with newer requirements is there and library users should guard against this by pinning their dependencies. In my experience it is much easier to down pin in case a dependency has to loose requirements than convincing your dependency solver the other way around (that a dependency is indeed too strict). |
|
Any updates here? I'm happy to invest a bit of time and rebase again and can also help settings something up for releases, but I think right now the process is manual and probably internal to you. |
mahrz24
force-pushed
the
loosen-requirements
branch
from
743d200 to
875fdbe
Compare
|
Sure, I just rebased and fixed the small conflict. |
|
@mahrz24 I'm rebasing your PR on main and updating a few of the dependencies, will aim to get this merged today |
|
@picklelo Thanks, if there are any problems, I can continue to look into it next week after the holidays. |
Also adds a import try except block for pydantic.v1 and relocks. Keep black and ruff to not mess to much with current formatting Make pyright see the right import as long as constraint still lock pydantiv v1 Down pin pytest-asyncio again due to known issue Fix upload handler with latest versions of fastapi Change comment
picklelo
force-pushed
the
loosen-requirements
branch
from
2cbb407 to
852a306
Compare
All Submissions:
Type of change
Removal of most upper bounds in the requirements. I also had to increase the lower bound of starlette-admin as the way the auth provider is passed has changed.
Yes, it passes all unit tests with small modifications. Locally the app harness test fail with timeouts (but run through when triggered individually), hence I would like to use this PR to run the integration tests in the same environment they usually run.
This so far is a draft and I am happy about any feedback. I would also add to the pipeline to run tests with latest versions (i.e. create a new lock file and in case it differs, rerun tests) on the main branch.
Closes #2777