-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
build: initial support for poetry build tool #4513
Conversation
How to guarantee the |
Hi @bowenliang123 , in current stage. we can use In the future, I'd like to suggest that remove |
Can you figure out at least one way in CI jobs to making sure I'm against setting the removal the
|
api/.python-version
Outdated
@@ -0,0 +1 @@ | |||
3.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is unnecessary, as the pyporject.toml
has set the required minimum python version.
Also, dify API module is tested on Python 3.10
and 3.11
in CI jobs.
If this is for local development only, please do not submit to the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem you listed is not caused by python 3.11 , I think.
And it should be fixed in #4517, bumping yfinance
on minor verions.
Please rebase to the latest commit and check again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an perfect example for inconsistency of your poetry dependencies and the required dependencies from requirements.txt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
This PR is just for providing another for developers to manage dependencies, it doesn't impact unit tests, CI and product env. As for your objections, I'd like to discuss with you in another PR(not created yet). |
The introduced support for a new build system should at least respect and align with the existing core mechanisms. Otherwise, it will puzzle the users with the issues caused by inconsistencies in dependencies(which happen often), as the de-facto two different dependencies are listed in the Dify API module. |
Currently, the only supported build system is still not changed. the dependencies in |
Hi @bowenliang123, is it possible to migrate to poetry in the future? Actually, currently lint and test are using poetry's configuration file, while package dependencies are using pip. I hope in the future we can consolidate everything with poetry, and poetry can parallelize the dependency fetching process to speed up package installation. |
Hi @takatost , I am always open to modern build systems like hatch or poetry. All I want to prevent is the inconsistency of dependencies, especially we don't have our decisions in the choice of build system. |
Support
[W. Logan C - Chat @ Spike](https://spikenow.com/r/a/?ref=spike-organic-signature&_ts=2lrqyj) [2lrqyj]
On May 20, 2024 at 13:52 GMT, langgenius/dify ***@***.***> wrote:
Hi ***@***.***(https://github.com/bowenliang123), is it possible to migrate to poetry in the future? Actually, currently lint and test are using poetry's configuration file, while package dependencies are using pip. I hope in the future we can consolidate everything with poetry, and poetry can parallelize the dependency fetching process to speed up package installation.
Hi ***@***.***(https://github.com/takatost) , I am always open to modern build system like hatch or poetry. All I want to prevent is the inconsistency of dependencies.
—
Reply to this email directly, [view it on GitHub](#4513 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BH6TQ24SXBJKPMDVNWIPNATZDH5ZFAVCNFSM6AAAAABH6OKP7CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRQGUYDMOJYGI).
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
?
[W. Logan C - Chat @ Spike](https://spikenow.com/r/a/?ref=spike-organic-signature&_ts=2lrqze) [2lrqze]
On May 20, 2024 at 14:22 GMT, W Logan C ***@***.***> wrote:
Support
[W. Logan C - Chat @ Spike](https://spikenow.com/r/a/?ref=spike-organic-signature&_ts=2lrqyj) [2lrqyj]
On May 20, 2024 at 13:52 GMT, langgenius/dify ***@***.***> wrote:
Hi ***@***.***(https://github.com/bowenliang123), is it possible to migrate to poetry in the future? Actually, currently lint and test are using poetry's configuration file, while package dependencies are using pip. I hope in the future we can consolidate everything with poetry, and poetry can parallelize the dependency fetching process to speed up package installation.
Hi ***@***.***(https://github.com/takatost) , I am always open to modern build system like hatch or poetry. All I want to prevent is the inconsistency of dependencies.
—
Reply to this email directly, [view it on GitHub](#4513 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/BH6TQ24SXBJKPMDVNWIPNATZDH5ZFAVCNFSM6AAAAABH6OKP7CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRQGUYDMOJYGI).
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
Langgenius is also GitHub!
Thank you
[W. Logan C - Chat @ Spike](https://spikenow.com/r/a/?ref=spike-organic-signature&_ts=2lrv1y) [2lrv1y]
|
I'm suggesting the following changes in this PR to meet the baseline of acceptance for initial support for
These actions still don't guarantee the poetry dependencies aligned to |
It's ok for me. But I'd like to get opinion from @takatost |
a77ade4
to
ac63620
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. It covers the tests with dependencies installed by peotry
. cc @takatost
Minor optional suggestions:
- use
poetry
style version specification like^
rather than>=
+<
for readability and maintainability - use
actions-poetry
v3 - basic info of [tool.poetry] should be updated as
- name: dify-api
- version: 0.6.8
- authors: Dify with its official email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
api/pyproject.toml
Outdated
|
||
[tool.poetry] | ||
name = "dify-api" | ||
version = "0.6.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version = "0.6.8" | |
version = "0.6.10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. It's best to update to the latest dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
api/pyproject.toml
Outdated
pytest-benchmark = "^4.0.0" | ||
pytest-env = "^1.1.3" | ||
pytest-mock = "^3.14.0" | ||
jinja2 = "^3.1.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jinjia2 has been removed from development dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
Co-authored-by: Bowen Liang <bowenliang@apache.org>
74c57ad
to
3c9f51a
Compare
Co-authored-by: Bowen Liang <bowenliang@apache.org>
Co-authored-by: Bowen Liang <bowenliang@apache.org>
Description
This PR is just adding poetry configurations in
pyproject.toml
so that we can use poety to easily manage the environment, as well as a little of README.md.Fixes # (issue)
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
This change doesn't bring out any changes of code, and I have tested with generic poetry commands.
Suggested Checklist:
dev/reformat
(backend) andcd web && npx lint-staged
(frontend) to appease the lint godsoptional
I have made corresponding changes to the documentationoptional
I have added tests that prove my fix is effective or that my feature worksoptional
New and existing unit tests pass locally with my changes