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

Backport 3046 - Pin grpc #3055

Merged
merged 1 commit into from
Jan 13, 2025
Merged

Backport 3046 - Pin grpc #3055

merged 1 commit into from
Jan 13, 2025

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Jan 13, 2025

#3046

Summary by Bito

This PR implements GRPC version pinning to address compatibility issues, enhances ArrayNode functionality for ReferenceTasks, and improves task resolution handling. The changes include refactoring of file system operations, local cache handling, and structured dataset management. The PR also adds comprehensive test coverage for VSCode integration and Polars structured dataset handling.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 5

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 13, 2025

Code Review Agent Run #da7ff6

Actionable Suggestions - 0
Additional Suggestions - 10
  • flytekit/interactive/vscode_lib/decorator.py - 1
    • Consider adding error handling for settings.json · Line 340-342
  • plugins/flytekit-polars/tests/test_polars_plugin_sd.py - 1
  • flytekit/extend/backend/agent_service.py - 1
    • Consider adding error type information · Line 67-67
  • tests/flytekit/unit/cli/pyflyte/test_run.py - 1
    • Consider more precise output assertion check · Line 480-480
  • plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_vscode.py - 1
    • Consider reordering decorators for proper wrapping · Line 428-429
  • pyproject.toml - 1
    • Consider adding upper bound for flyteidl · Line 23-23
  • flytekit/core/array_node.py - 1
    • Consider adding data_mode property validation · Line 140-141
  • flytekit/types/structured/__init__.py - 1
    • Consider extracting repeated handler registration pattern · Line 89-110
  • flytekit/core/context_manager.py - 1
    • Consider more modular task resolver initialization · Line 721-723
  • tests/flytekit/unit/core/test_data_persistence.py - 1
    • Consider parameterizing similar test cases · Line 219-221
Review Details
  • Files reviewed - 36 · Commit Range: 03cec6b..ba9892d
    • Dockerfile.agent
    • flytekit/core/array_node.py
    • flytekit/core/array_node_map_task.py
    • flytekit/core/context_manager.py
    • flytekit/core/data_persistence.py
    • flytekit/core/local_cache.py
    • flytekit/core/tracker.py
    • flytekit/core/type_engine.py
    • flytekit/core/workflow.py
    • flytekit/extend/backend/agent_service.py
    • flytekit/interactive/vscode_lib/decorator.py
    • flytekit/lazy_import/lazy_module.py
    • flytekit/loggers.py
    • flytekit/models/core/workflow.py
    • flytekit/models/literals.py
    • flytekit/remote/remote.py
    • flytekit/tools/translator.py
    • flytekit/types/structured/__init__.py
    • flytekit/types/structured/structured_dataset.py
    • plugins/flytekit-airflow/setup.py
    • plugins/flytekit-dbt/setup.py
    • plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_vscode.py
    • plugins/flytekit-inference/flytekitplugins/inference/ollama/serve.py
    • plugins/flytekit-polars/flytekitplugins/polars/sd_transformers.py
    • plugins/flytekit-polars/tests/test_polars_plugin_sd.py
    • pyproject.toml
    • tests/flytekit/integration/remote/test_remote.py
    • tests/flytekit/unit/cli/pyflyte/test_run.py
    • tests/flytekit/unit/core/test_data_persistence.py
    • tests/flytekit/unit/core/test_generice_idl_type_engine.py
    • tests/flytekit/unit/core/test_local_cache.py
    • tests/flytekit/unit/core/test_type_engine.py
    • tests/flytekit/unit/core/test_workflows.py
    • tests/flytekit/unit/lazy_module/test_lazy_module.py
    • tests/flytekit/unit/remote/test_remote.py
    • tests/flytekit/unit/types/structured_dataset/test_structured_dataset.py
  • Files skipped - 2
    • .github/workflows/pythonbuild.yml - Reason: Filter setting
    • .github/workflows/pythonpublish.yml - Reason: Filter setting
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@wild-endeavor wild-endeavor changed the base branch from master to release-v1.14 January 13, 2025 20:23
@wild-endeavor wild-endeavor merged commit 17841c8 into release-v1.14 Jan 13, 2025
26 of 27 checks passed
@flyte-bot
Copy link
Contributor

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - GRPC Version Pinning and Dependency Updates

pyproject.toml - Pin grpcio and grpcio-status versions to address compatibility issues

Dockerfile.agent - Reorganize pip installation of prometheus-client and grpcio-health-checking

setup.py - Add version constraint for apache-airflow-providers-google

setup.py - Update dbt-core version requirements and add networkx dependency

Feature Improvement - Array Node and Task Resolution Enhancements

array_node.py - Add support for ReferenceTask in ArrayNode and improve execution mode handling

workflow.py - Enhance task resolver handling in workflow compilation

context_manager.py - Update compilation state to use default task resolver

Other Improvements - Code Optimization and Refactoring

data_persistence.py - Simplify file system operations by removing async handling

local_cache.py - Improve literal map handling and backwards compatibility

lazy_module.py - Refactor lazy module implementation for better import handling

__init__.py - Reorganize structured dataset handler registration

remote.py - Enhance workflow and launch plan registration logic

Testing - Test Coverage Improvements

test_flyteinteractive_vscode.py - Add test coverage for VSCode integration with dynamic tasks

test_polars_plugin_sd.py - Add tests for Polars structured dataset with URI handling

Bug Fix - GRPC Version Pinning and Dependency Updates

pyproject.toml - Pin grpcio and grpcio-status versions to address compatibility issues

Dockerfile.agent - Reorganize pip installation of prometheus-client and grpcio-health-checking

setup.py - Add version constraint for apache-airflow-providers-google

setup.py - Update dbt-core version requirements and add networkx dependency

Testing - Test Suite Enhancements

test_local_cache.py - Add tests for cache handling of old literal map versions

test_data_persistence.py - Add tests for raw data bytes handling

test_lazy_module.py - Enhance lazy module import testing

test_remote.py - Add tests for task registration with node dependency hints

test_workflows.py - Add test for workflow LHS property

test_remote.py - Add test for launch plan registration

Other Improvements - Code Cleanup and Optimization

test_type_engine.py - Refactor concurrent transformer import tests

test_run.py - Update assertion logic for execution output

test_generice_idl_type_engine.py - Remove redundant test cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants