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.

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

fix: add tests to cycles in Graph and improve error handling #3628

Merged
merged 7 commits into from
Sep 2, 2024

Conversation

ogabrielluiz
Copy link
Contributor

This pull request adds cycle detection and handling functionality to the graph processing module. It introduces the is_cyclic property to check for cycles in the graph and modifies the layered_topological_sort method to handle cyclic graphs by starting from a specified start component. Additionally, it refactors tests and components for improved caching and data handling, updates the VertexStates enum values to uppercase, and improves error handling in the Vertex class. Unit tests for detecting cycles in the graph are also included.

- Introduced `cycles` property to detect cycles in the graph.
- Modified `_build_edges` and `build_edge` methods to differentiate between `CycleEdge` and `Edge`.
- Updated imports and type hints to support new functionality.
- Introduced `is_cyclic` property to check for cycles in the graph.
- Added `_snapshot` method for capturing the current state of the graph.
- Modified `layered_topological_sort` to handle cyclic graphs by starting from a specified start component.
- Updated imports and type hints for better code clarity and functionality.
- Updated `test_vector_store_rag.py` to use `set_on_output` with `cache=True` and simplified assertions.
- Enhanced `test_memory_chatbot.py` with additional assertions for graph structure and caching.
- Simplified `to_data` method in `base.py` to directly return `_data` without JSON serialization.
- Introduce `test_cycle_in_graph` to verify cyclic behavior in the graph.
- Add `test_cycle_in_graph_max_iterations` to ensure max iterations limit is respected.
- Implement `Concatenate` component for testing purposes.
- Replace `ValueError` with `NoComponentInstance` exception for missing component instances.
- Add `target_handle_name` parameter to `_get_result` method for better result retrieval.
- Refactor type hints to use `collections.abc` for `AsyncIterator`, `Generator`, and `Iterator`.
- Update type hints for `extract_messages_from_artifacts` and `successors_ids` methods to use generic `dict` and `list`.
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Aug 30, 2024
@github-actions github-actions bot added bug Something isn't working and removed enhancement New feature or request labels Aug 30, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-3628.dmtpw4p5recq1.amplifyapp.com

Copy link
Contributor

@nicoloboschi nicoloboschi 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 Sep 2, 2024
@ogabrielluiz ogabrielluiz enabled auto-merge (squash) September 2, 2024 11:31
@ogabrielluiz ogabrielluiz enabled auto-merge (squash) September 2, 2024 11:43
@ogabrielluiz ogabrielluiz merged commit bc6e918 into main Sep 2, 2024
38 of 46 checks passed
@ogabrielluiz ogabrielluiz deleted the fix/cycles branch September 2, 2024 11:45
carlosrcoelho pushed a commit that referenced this pull request Sep 2, 2024
* Add cycle detection and handling in graph edge building process

- Introduced `cycles` property to detect cycles in the graph.
- Modified `_build_edges` and `build_edge` methods to differentiate between `CycleEdge` and `Edge`.
- Updated imports and type hints to support new functionality.

* Add cycle detection and handling in graph processing

- Introduced `is_cyclic` property to check for cycles in the graph.
- Added `_snapshot` method for capturing the current state of the graph.
- Modified `layered_topological_sort` to handle cyclic graphs by starting from a specified start component.
- Updated imports and type hints for better code clarity and functionality.

* Refactor tests and components for improved caching and data handling

- Updated `test_vector_store_rag.py` to use `set_on_output` with `cache=True` and simplified assertions.
- Enhanced `test_memory_chatbot.py` with additional assertions for graph structure and caching.
- Simplified `to_data` method in `base.py` to directly return `_data` without JSON serialization.

* Add unit tests for detecting cycles in graph

- Introduce `test_cycle_in_graph` to verify cyclic behavior in the graph.
- Add `test_cycle_in_graph_max_iterations` to ensure max iterations limit is respected.
- Implement `Concatenate` component for testing purposes.

* Disable output cache in graph tests to allow loops to work

* Refactor: Update VertexStates enum values to uppercase and optimize imports in base.py

* Refactor type hints and improve error handling in `Vertex` class

- Replace `ValueError` with `NoComponentInstance` exception for missing component instances.
- Add `target_handle_name` parameter to `_get_result` method for better result retrieval.
- Refactor type hints to use `collections.abc` for `AsyncIterator`, `Generator`, and `Iterator`.
- Update type hints for `extract_messages_from_artifacts` and `successors_ids` methods to use generic `dict` and `list`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants