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 raw container (copilot) integration test #2892

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wayner0628
Copy link
Contributor

Tracking issue

flyteorg/flyte#5930

Why are the changes needed?

To prevent in the future, someone writing code that will break the copilot.

What changes were proposed in this pull request?

Add code tests/flytekit/unit/core/test_local_raw_container.py to tests/flytekit/integration/remote/test_remote.py.

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

#2887

Docs link

Signed-off-by: wayner0628 <a901639@gmail.com>
@wayner0628 wayner0628 changed the title add raw container integration test Add raw container (copilot) integration test Nov 3, 2024
Signed-off-by: wayner0628 <a901639@gmail.com>
Signed-off-by: wayner0628 <a901639@gmail.com>
Copy link

codecov bot commented Nov 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.94%. Comparing base (6bf6f8e) to head (6efafae).
Report is 6 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (6bf6f8e) and HEAD (6efafae). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (6bf6f8e) HEAD (6efafae)
4 3
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2892       +/-   ##
===========================================
- Coverage   76.79%   43.94%   -32.86%     
===========================================
  Files         196      199        +3     
  Lines       20546    20781      +235     
  Branches     2646     2671       +25     
===========================================
- Hits        15779     9132     -6647     
- Misses       4068    11416     +7348     
+ Partials      699      233      -466     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: wayner0628 <a901639@gmail.com>
@Future-Outlier Future-Outlier self-assigned this Nov 4, 2024
@wayner0628
Copy link
Contributor Author

wayner0628 commented Nov 4, 2024

The current issue with this PR is that the integration test environment is unable to locate the Docker server needed to build the copilot image.

Not sure if it is this

@pytest.mark.skipif(
    sys.platform in ["darwin", "win32"],
    reason="Skip if running on windows or macos due to CI Docker environment setup failure",
)

makes the unittest environment able to pass the test

@wayner0628
Copy link
Contributor Author

I tried using subprocess to build the image, but there's no docker command

line 22, in build_docker_image
    subprocess.run(
      File "/usr/local/lib/python3.9/subprocess.py", line 505, in run
    with Popen(*popenargs, **kwargs) as process:
      File "/usr/local/lib/python3.9/subprocess.py", line 951, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
      File "/usr/local/lib/python3.9/subprocess.py", line 1837, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)

Message:

    FileNotFoundError: [Errno 2] No such file or directory: 'docker'

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Comment on lines +13 to +19

client = docker.from_env()
path_to_dockerfile = "tests/flytekit/unit/core/"
dockerfile_name = "Dockerfile.raw_container"
client.images.build(path=path_to_dockerfile, dockerfile=dockerfile_name, tag="flytekit:rawcontainer")

flyte_file_io = ContainerTask(
Copy link
Member

Choose a reason for hiding this comment

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

I think the only way to do this is to build an image at CI.
We can use my image futureoutlier/rawcontainer:0320 first.

Copy link
Member

Choose a reason for hiding this comment

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

cc @pingsutw what do you think to build your image when executing integration test?
Are these 2 the only ways?

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

Successfully merging this pull request may close these issues.

2 participants