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.

Sign up for GitHub

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

Jump to bottom

chore: refactor the http executor node #5212

Merged
merged 15 commits into from
Jun 24, 2024
Merged

chore: refactor the http executor node #5212

merged 15 commits into from
Jun 24, 2024

Conversation

hjlarry
Copy link
Contributor

@hjlarry hjlarry commented Jun 14, 2024

Description

Change List:

  1. remove the requests of ssrf_proxy. In the past, we use both httpx and requests because it support body of DELETE method. Now we can use httpx to achieve it too. more details here
  2. remove the OPTION method of backend, and make the GET / DELETE / HEAD also support http body. This maintains the consistency between the front-end and back-end. In the past , we implicit drop the body of GET.
  3. upgrade httpx from 0.24.1 to 0.27.0. the release note seems not to affect our usage scenarios. It support Python3.12 which maybe we will support later, and the proxies parameter will be deprecate later, use a singleproxy for all request maybe more commonly used.
  4. drop *args of ssrf_proxy request methods, cause httpx only support **kwargs.
  5. refactor the _to_dict method which convert params/headers/body from frontend to python dict, and add test for it.

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update, included: Dify Document
  • Improvement, including but not limited to code refactoring, performance optimization, and UI/UX improvement
  • Dependency upgrade

How Has This Been Tested?

Integration test and local test.

  • TODO

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods
  • optional I have made corresponding changes to the documentation
  • optional I have added tests that prove my fix is effective or that my feature works
  • optional New and existing unit tests pass locally with my changes

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. dependencies Pull requests that update a dependency file 🐍 python 💪 enhancement New feature or request labels Jun 14, 2024
@hjlarry
Copy link
Contributor Author

hjlarry commented Jun 14, 2024

@Yeuoly @bowenliang123 please review it, thanks

@crazywoola crazywoola requested a review from Yeuoly June 14, 2024 10:46
@Yeuoly
Copy link
Collaborator

Yeuoly commented Jun 15, 2024

Hi, thanks a lot for the refactoring! but there are still some parts that need improvement. Specifically, in the core.tools.tool.api_tool, the do_http_request and validate_and_parse_response methods also use ssrf_proxy. I intend to refactor these as well. how to you think of it?

@bowenliang123
Copy link
Contributor

+1 for refactoring the HTTP requesting both in HTTP executor node and in API tools, unifying into using httpx.

@bowenliang123
Copy link
Contributor

bowenliang123 commented Jun 15, 2024 •
edited
Loading

BTW, please rebase the changes onto the latest commits. Update the poetry dependencies in pyproject.yml and poetry lockfile as well (running dev/sync-poetry).

@crazywoola
Copy link
Member

Please fix the CI

@hjlarry
Copy link
Contributor Author

hjlarry commented Jun 17, 2024

Please fix the CI

as this PR(#5319) merged will fix the CI

@bowenliang123
Copy link
Contributor

Wait for #5319 fixing the problem merged, and then rebase onto it.

@crazywoola
Copy link
Member

crazywoola commented Jun 17, 2024 •
edited
Loading

#5319 Fixed @hjlarry

@hjlarry
Copy link
Contributor Author

hjlarry commented Jun 18, 2024

Hi, thanks a lot for the refactoring! but there are still some parts that need improvement. Specifically, in the core.tools.tool.api_tool, the do_http_request and validate_and_parse_response methods also use ssrf_proxy. I intend to refactor these as well. how to you think of it?

done! please review again. @Yeuoly @bowenliang123

@bowenliang123
Copy link
Contributor

LGTM overall.

Yeuoly
Yeuoly approved these changes Jun 24, 2024
View reviewed changes
Copy link
Collaborator

@Yeuoly Yeuoly left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 24, 2024
@Yeuoly Yeuoly merged commit f7900f2 into langgenius:main Jun 24, 2024
5 checks passed
@Yeuoly
Copy link
Collaborator

Yeuoly commented Jun 24, 2024

Thanks for your great job!

ZhouhaoJiang added a commit that referenced this pull request Jun 24, 2024
@ZhouhaoJiang
Merge branch 'refs/heads/main' into feat/cleaning_ops_trace
c885fe0 
* refs/heads/main: (31 commits)
  feat: support predefined models for openrouter (#5494)
  chore: refactor the http executor node (#5212)
  chore: add create_json_message api for tools (#5440)
  fix(api/model_runtime/azure/llm): Switch to tool_call. (#5541)
  Fix: position of log modal (#5538)
  feat: Add program_name attribute to TiDB connection (#5499)
  add opensearch default value (#5536)
  feat: make Citations and Attributions display enable default (#5508)
  Fix: resolve issue with embedding model field visibility toggling on datasets page (#5451)
  Fix: custom disclaim (#5535)
  Chore: chat log refactor (#5523)
  feat: use root dir to start python and celery (#5515)
  feat: add new features to enhance image and link handling in Jina tool (#5517)
  fix: apply best practices for the latest buildkit (#5527)
  fix: added error handling for novita ai tool query (#5506)
  feat: add Asia/Ho_Chi_Minh timezone (#5521)
  fix: correct typos (#5510)
  chore: use singular style in middleware config class name (#5502)
  refactor: extract hosted service configs into dify config (#5504)
  fix: summary of duckduckgo_search (#5488)
  ...

# Conflicts:
#	api/core/app/apps/advanced_chat/app_generator.py
#	api/poetry.lock
#	api/requirements.txt
ZhouhaoJiang added a commit that referenced this pull request Jun 25, 2024
@ZhouhaoJiang
Merge branch 'refs/heads/feat/cleaning_ops_trace' into deploy/dev
c47c00c 
* refs/heads/feat/cleaning_ops_trace: (21 commits)
  feat: add provider_config_map
  feat: update poetry.lock
  update WorkflowNodeExecution init created_at
  chore: change ops_trace_service location
  feat: support predefined models for openrouter (#5494)
  chore: refactor the http executor node (#5212)
  chore: add create_json_message api for tools (#5440)
  fix(api/model_runtime/azure/llm): Switch to tool_call. (#5541)
  Fix: position of log modal (#5538)
  feat: Add program_name attribute to TiDB connection (#5499)
  add opensearch default value (#5536)
  feat: make Citations and Attributions display enable default (#5508)
  Fix: resolve issue with embedding model field visibility toggling on datasets page (#5451)
  Fix: custom disclaim (#5535)
  Chore: chat log refactor (#5523)
  feat: use root dir to start python and celery (#5515)
  feat: add new features to enhance image and link handling in Jina tool (#5517)
  fix: apply best practices for the latest buildkit (#5527)
  fix: added error handling for novita ai tool query (#5506)
  feat: add Asia/Ho_Chi_Minh timezone (#5521)
  ...

# Conflicts:
#	api/core/ops/model.py
#	api/poetry.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yeuoly Yeuoly Yeuoly approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file 💪 enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hjlarry @Yeuoly @bowenliang123 @crazywoola