-
-
Notifications
You must be signed in to change notification settings - Fork 0
This PR represents a major API and architecture refactoring of the thinkagain framework, introducing a cleaner module structure, updated terminology, and improved runtime capabilities. #61
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
Conversation
The agents module has been removed as part of the refactoring to rename core concepts.
Add __init__.pyi with comprehensive type annotations for the public API.
- Rename NodeExecutionError to OpExecutionError - Add context.py for execution context - Add op.py for operation handling - Update various runtime files to use new naming - Remove optimizer.py and task.py as they are no longer needed
- Rename replica.proto to service.proto and regenerate protobuf files - Update gRPC backend code to use service terminology - Update service provider code accordingly
- Update mesh.py and __init__.py to use new terminology - Add require_mesh function for mesh requirement checking
- Rename @node decorator to @op in documentation and examples - Rename @replica decorator to @service in architecture docs and tests - Update README.md to reflect dynamic execution model instead of JAX-style compilation - Update DESIGN.md to use ObjectRef terminology instead of Context - Update test comments and file headers to use new decorator names - Remove outdated agents example documentation
Merging this PR will degrade performance by 30.78%
Performance Changes
Comparing Footnotes
|
Summary of ChangesHello @BDHU, 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 introduces a fundamental architectural shift in the 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d204bf5050
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _unique_dependencies(self, op: Op) -> set[ObjectRef]: | ||
| """Return unique ObjectRef dependencies for an op.""" | ||
| return set(op.get_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.
Avoid hashing ObjectRef dependencies
When an op depends on another op, op.get_dependencies() returns ObjectRef instances. _unique_dependencies builds a set from that list, but ObjectRef is a mutable dataclass without __hash__, so this raises TypeError: unhashable type: 'ObjectRef' as soon as any dependency exists. That means any .go() call that passes an ObjectRef argument will fail during submission. Consider deduping by op_id (or another hashable key) instead of putting ObjectRef objects directly in a set.
Useful? React with 👍 / 👎.
| return { | ||
| "pending": max(len(self._tasks) - len(self._running), 0), | ||
| "running": len(self._running), |
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.
Fix stats pending count to use tracked ops
The stats property references self._tasks, but the scheduler tracks ops in self._ops and never defines _tasks. Accessing scheduler.stats will therefore raise AttributeError, breaking any monitoring or tests that call it. This looks like a leftover rename and should be updated to use the correct collection.
Useful? React with 👍 / 👎.
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 represents a significant and well-executed refactoring of the thinkagain framework. The transition from a JAX-style static graph compilation to a dynamic execution model using @op, @service, and .go() calls introduces a much cleaner API and improved runtime capabilities. The module restructuring into api, backends, resources, and runtime is logical and enhances maintainability. All documentation, examples, and tests have been thoroughly updated to reflect the new terminology and architectural changes. The introduction of ObjectRef and DAGScheduler as core components for dynamic dependency tracking and parallel execution is a strong improvement. Overall, this is a comprehensive and well-implemented refactoring that significantly enhances the framework's design and usability.
Key Changes
API Renaming
Module Restructuring
New Features
Documentation
Tests & Examples
Breaking Changes: This is a significant refactoring with API renames. Users will need to update their code: