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

Add poetry to image spec #3025

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Dec 30, 2024

Tracking issue

Related to flyteorg/flyte#6119

Why are the changes needed?

This allows poetry uses to create a image spec with their dependencies. This PR treats poetry as a "dependency manager". This means that their local project will not be installed. If we see that poetry users want to install their local package, then we can extend this feature with a follow up PR.

What changes were proposed in this pull request?

This PR adds a requirements="poetry.lock" that creates an ImageSpec with poetry.

How was this patch tested?

I created a repo with poetry init with this workflow:

from flytekit import task, ImageSpec
import numpy as np
from random import choice
from string import ascii_letters


image = ImageSpec(
    name="numpy-poetry",
    requirements="poetry.lock",
    registry="localhost:30000",
    env={"A": "".join(choice(ascii_letters) for _ in range(20))},
)


@task(container_image=image)
def add_task() -> np.ndarray:
    x = np.ones(10)
    return x + 10

and ran pyflyte run --remote workflows/main.py.

Summary by Bito

This PR implements Poetry support in ImageSpec with lock file handling capabilities and Docker build integration. It enhances array node capabilities with ReferenceTask support and improves file handling for remote access. The implementation includes comprehensive error handling across components, particularly in file operations and logging systems. Changes include expanded testing infrastructure with improved variable naming and VSCode integration enhancements.

Unit tests added: True

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

Signed-off-by: Thomas J. Fan <[email protected]>
@flyte-bot
Copy link
Contributor

flyte-bot commented Dec 30, 2024

Code Review Agent Run #73368b

Actionable Suggestions - 5
  • tests/flytekit/unit/core/image_spec/test_default_builder.py - 2
    • Consider renaming shadowed lock_file variable · Line 281-281
    • Consider consolidating duplicate pytest parametrize decorator · Line 275-275
  • flytekit/image_spec/default_builder.py - 3
Review Details
  • Files reviewed - 2 · Commit Range: 0813ae8..0813ae8
    • flytekit/image_spec/default_builder.py
    • tests/flytekit/unit/core/image_spec/test_default_builder.py
  • Files skipped - 0
  • 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

@flyte-bot
Copy link
Contributor

flyte-bot commented Dec 30, 2024

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Poetry Support for ImageSpec

default_builder.py - Added Poetry lock file support and template for Docker builds

test_default_builder.py - Added test cases for Poetry lock file handling and error scenarios

run_mock = Mock()
monkeypatch.setattr("flytekit.image_spec.default_builder.run", run_mock)

uv_lock_file = tmp_path / "uv.lock"
uv_lock_file.write_text("this is a lock file")
lock_file = tmp_path / lock_file
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming shadowed lock_file variable

The variable name lock_file is being redefined in the function scope, shadowing the parameter with the same name. This could lead to confusion and potential bugs.

Code suggestion
Check the AI-generated fix before applying
Suggested change
lock_file = tmp_path / lock_file
lock_file_path = tmp_path / lock_file

Code Review Run #73368b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

builder.build_image(image_spec)


@pytest.mark.parametrize("lock_file", ["uv.lock", "poetry.lock"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider consolidating duplicate pytest parametrize decorator

Consider consolidating the duplicate @pytest.mark.parametrize decorator with the one at line 254 since both test functions use the same parameters.

Code suggestion
Check the AI-generated fix before applying
 @@ -254,7 +254,6 @@
  @pytest.mark.parametrize("lock_file", ["uv.lock", "poetry.lock"])
  @pytest.mark.filterwarnings("ignore::UserWarning")
  def test_lock_errors_no_pyproject_toml(monkeypatch, tmp_path, lock_file):

  @@ -274,7 +274,6 @@
 [email protected]("lock_file", ["uv.lock", "poetry.lock"])
  @pytest.mark.filterwarnings("ignore::UserWarning")
  def test_uv_lock_error_no_packages(monkeypatch, tmp_path, lock_file):

Code Review Run #73368b


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

Signed-off-by: Thomas J. Fan <[email protected]>
@flyte-bot
Copy link
Contributor

flyte-bot commented Dec 30, 2024

Code Review Agent Run #edb466

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 0813ae8..b32a4df
    • tests/flytekit/unit/core/image_spec/test_default_builder.py
  • Files skipped - 0
  • 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

@flyte-bot
Copy link
Contributor

flyte-bot commented Jan 3, 2025

Code Review Agent Run #9afb0f

Actionable Suggestions - 0
Additional Suggestions - 7
  • flytekit/clis/sdk_in_container/run.py - 1
    • Consider consolidating JSON serialization logic · Line 478-486
  • flytekit/core/array_node.py - 1
    • Consider adding data mode validation check · Line 140-141
  • tests/flytekit/unit/cli/pyflyte/test_run.py - 2
  • tests/flytekit/integration/remote/test_remote.py - 2
    • Consider using unittest.mock instead of mock · Line 17-17
    • Consider adding cleanup error handling · Line 832-835
  • tests/flytekit/unit/bin/test_python_entrypoint.py - 1
    • Consider consolidating repeated timestamp initialization pattern · Line 529-540
Review Details
  • Files reviewed - 28 · Commit Range: b32a4df..fa746c8
    • dev-requirements.txt
    • flytekit/clis/sdk_in_container/run.py
    • flytekit/core/array_node.py
    • flytekit/core/array_node_map_task.py
    • flytekit/core/context_manager.py
    • flytekit/core/local_cache.py
    • flytekit/core/tracker.py
    • flytekit/core/workflow.py
    • flytekit/extend/backend/agent_service.py
    • flytekit/extend/backend/base_agent.py
    • flytekit/interactive/vscode_lib/decorator.py
    • flytekit/loggers.py
    • flytekit/models/core/workflow.py
    • flytekit/models/literals.py
    • flytekit/tools/translator.py
    • flytekit/types/directory/types.py
    • flytekit/types/file/file.py
    • plugins/flytekit-airflow/setup.py
    • plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_vscode.py
    • pyproject.toml
    • tests/flytekit/integration/remote/test_remote.py
    • tests/flytekit/integration/remote/utils.py
    • tests/flytekit/integration/remote/workflows/basic/flytefile.py
    • tests/flytekit/integration/remote/workflows/basic/pydantic_wf.py
    • tests/flytekit/unit/bin/test_python_entrypoint.py
    • tests/flytekit/unit/cli/pyflyte/test_run.py
    • tests/flytekit/unit/core/test_local_cache.py
    • tests/flytekit/unit/types/directory/test_listdir.py
  • Files skipped - 1
    • .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

@thomasjpfan thomasjpfan merged commit 87ac7f7 into flyteorg:master Jan 6, 2025
102 checks passed
granthamtaylor added a commit that referenced this pull request Jan 6, 2025
* Store protos in local cache (#3022)

* Store proto obj instead of model Literal in local cache

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove unused file

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Bump aiohttp from 3.9.5 to 3.10.11 (#3018)

Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.9.5 to 3.10.11.
- [Release notes](https://github.com/aio-libs/aiohttp/releases)
- [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst)
- [Commits](aio-libs/aiohttp@v3.9.5...v3.10.11)

---
updated-dependencies:
- dependency-name: aiohttp
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix bug in FlyteDirectory.listdir on local files (#2926)

* Fix issue in FlyteDirectory.listdir

Fixes flyteorg/flyte#6005

Signed-off-by: Pim de Haan <[email protected]>

* Added test

Signed-off-by: Pim de Haan <[email protected]>

* Run make lint

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Pim de Haan <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>

* Fix unit tests in airflow plugin (#3024)

Signed-off-by: Kevin Su <[email protected]>

* fix: Fix resource meta typos for async agent (#3023)

Signed-off-by: JiaWei Jiang <[email protected]>

* fix: format commands output (#3026)

* Fix pydantic basemodel default input (#3013)

* Fix pydantic default input

Signed-off-by: Future-Outlier <[email protected]>

* add pydantic integration test

Signed-off-by: Future-Outlier <[email protected]>

* Use duck typing by Thomas's advice

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>

* lint

Signed-off-by: Future-Outlier <[email protected]>

---------

Signed-off-by: Future-Outlier <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>

* [BUG] Open FlyteFile from remote path (#2991)

* fix: Open FlyteFile from remote path

Signed-off-by: JiaWei Jiang <[email protected]>

* Add integration test

Signed-off-by: JiaWei Jiang <[email protected]>

* refactor: Use ctx as param instead of recreation

Signed-off-by: JiaWei Jiang <[email protected]>

* refactor: Clean test logic

1. Remove redundant prints
2. Use `mock.patch.dict` to setup `os.environ` for the current test fn
    * Avoid contaminating other tests running in the same process

Signed-off-by: JiaWei Jiang <[email protected]>

* refactor: Setup local path and downloader in constructor

Signed-off-by: JiaWei Jiang <[email protected]>

* refactor: Move SimpleFileTransfer to an utility file

Signed-off-by: JiaWei Jiang <[email protected]>

* Remove redundant env var setup

Please refer to #3001

Signed-off-by: JiaWei Jiang <[email protected]>

* test: Add another ff use case

Create ff in one task pod and read it in another task pod.

Signed-off-by: JiaWei Jiang <[email protected]>

---------

Signed-off-by: JiaWei Jiang <[email protected]>

* vllm inference plugin (#2967)

* vllm inference plugin

Signed-off-by: Daniel Sola <[email protected]>

* fixed default value

Signed-off-by: Daniel Sola <[email protected]>

---------

Signed-off-by: Daniel Sola <[email protected]>

* Add poetry to image spec (#3025)

* Add poetry to image spec

Signed-off-by: Thomas J. Fan <[email protected]>

* Add stricter check

Signed-off-by: Thomas J. Fan <[email protected]>

---------

Signed-off-by: Thomas J. Fan <[email protected]>

* [test] Add integration test for accessing sd sttr in dc (#2969)

* test: Add integration test for attr access of sd

Signed-off-by: JiaWei Jiang <[email protected]>

* Correct file path

Signed-off-by: JiaWei Jiang <[email protected]>

* test: Support interaction with minio s3 bucket

1. Upload a local parquet file to minio s3 bucket
2. Access StructuredDataset attr from a dataclass
3. Open StructuredDataset from a remote path

Signed-off-by: JiaWei Jiang <[email protected]>

* Delete an unmerged integration test

Signed-off-by: JiaWei Jiang <[email protected]>

* Try imagespec with commit sha of corresponding fix

Signed-off-by: JiaWei Jiang <[email protected]>

* Remove redundant test

Signed-off-by: JiaWei Jiang <[email protected]>

* Remove default_factory and create sd dc from input uri

Signed-off-by: JiaWei Jiang <[email protected]>

* refactor: Clean test logic

1. Remove redundant prints
2. Use `mock.patch.dict` to setup `os.environ` for the current test fn
    * Avoid contaminating other tests running in the same process

Signed-off-by: JiaWei Jiang <[email protected]>

* Remove redundant minio env var setup and add test comments

Signed-off-by: JiaWei Jiang <[email protected]>

* Support uploading tmp pqt file

Signed-off-by: JiaWei Jiang <[email protected]>

* Udpate deprecated module

Signed-off-by: JiaWei Jiang <[email protected]>

* Remove redundant and unused imports

Signed-off-by: JiaWei Jiang <[email protected]>

---------

Signed-off-by: JiaWei Jiang <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Pim de Haan <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: JiaWei Jiang <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Daniel Sola <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pim de Haan <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Co-authored-by: 江家瑋 <[email protected]>
Co-authored-by: V <[email protected]>
Co-authored-by: Han-Ru Chen (Future-Outlier) <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: Daniel Sola <[email protected]>
shuyingliang pushed a commit to shuyingliang/flytekit that referenced this pull request Jan 11, 2025
* Add poetry to image spec

Signed-off-by: Thomas J. Fan <[email protected]>

* Add stricter check

Signed-off-by: Thomas J. Fan <[email protected]>

---------

Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Shuying Liang <[email protected]>
@amitani amitani mentioned this pull request Jan 23, 2025
3 tasks
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.

3 participants