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

Changes default destination dir to be "working dir" #2108

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

kumare3
Copy link
Contributor

@kumare3 kumare3 commented Jan 16, 2024

Tracking issue

flyteorg/flyte#4638

Why are the changes needed?

In the absence of this change pyflyte run configures the default destination directory - the path where the code should be installed within a container to be /root. This does not work for cases in which the user may want a directory other than /root. Though it is possible to pass in this directory, it fails in case you have a task with multiple tasks, each uses a different container and separate paths.

A propose solution by @vkaiser-mb is to set it on a per task basis

@task(working_dir="/home/user_a", container_image="a")
def do_a():
 ...

@task(working_dir="/home/user_b", container_image="a")
def do_b():
  ...

@workflow
def wf():
  do_a()
  do_b()

This complicates the process, as it is one more argument on the task and is not simple to change and hard to keep in sync with the container Dockerfile.

What changes were proposed in this pull request?

The proposal is to instead simply set the WORKDIR in the Dockerfile and use that as the actual location of the installation. The code change then makes the current working dir as the default installation path.

How was this patch tested?

Tested with pyflyte run using existing flytekit dockerfile as it sets the WORKDIR correctly

WORKDIR /root
to /root We could easily change it to be non-root and this should continue to work.

This allows for using the default specified working dir in the docker file

Signed-off-by: Ketan Umare <kumare3@users.noreply.github.com>
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (892b474) 85.77% compared to head (87c15bf) 85.77%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2108   +/-   ##
=======================================
  Coverage   85.77%   85.77%           
=======================================
  Files         313      313           
  Lines       23500    23500           
  Branches     3512     3512           
=======================================
  Hits        20158    20158           
  Misses       2734     2734           
  Partials      608      608           

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

@kumare3 kumare3 merged commit 7996c2e into master Jan 17, 2024
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants