-
Notifications
You must be signed in to change notification settings - Fork 19.5k
refactor: refactor workflow context #30607
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @fatelei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the workflow context management by introducing an abstraction layer to decouple workflow execution from direct Flask context dependencies. This change aims to enhance the system's robustness, particularly in multi-threaded environments, and improve testability by allowing contexts to be explicitly managed and passed through dependency injection. The core idea is to move away from implicit global state to explicit context objects, making the codebase more maintainable and scalable. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a significant and valuable refactoring by abstracting the Flask context dependency within the workflow engine. The new ExecutionContext abstraction, along with its Flask-specific and null implementations, greatly improves the modularity, testability, and portability of the code. The changes are well-structured and the addition of unit tests for the new context management system is commendable.
I've identified a critical issue in _try_resolve_user_from_request that could lead to a RuntimeError, and a medium-severity issue regarding code duplication in FlaskExecutionContext. Addressing these will make the implementation more robust and maintainable.
Additionally, I noticed that app_generator.py still uses the old preserve_flask_contexts for its worker thread. While this is outside the scope of the changed lines in this PR, I recommend creating a follow-up task to refactor it to use the new ExecutionContext for consistency across the codebase.
fd111d3 to
e4ba676
Compare
api/core/workflow/graph_engine/worker_management/worker_pool.py
Outdated
Show resolved
Hide resolved
e4ba676 to
c4227a2
Compare
c4227a2 to
db6a7cd
Compare
6d9264a to
1924fb3
Compare
Important
Fixes #<issue number>.Summary
resolve #30595
add method
register_context_capturerto register context, later callcapture_current_contextto fetch the context,register_context_capturercan register many context, so workflow can use different context, to decouple the flask context, the workflow can not see the flask context.Screenshots
Checklist
make lintandmake type-check(backend) andcd web && npx lint-staged(frontend) to appease the lint gods