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

Run KFP container as local user/group if local #304

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

stefannica
Copy link
Contributor

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Describe changes

Some files and folders in the local repositories (e.g. database,
.system TFX directories) are shared between different runs of the
same pipeline or of different pipelines. If pipelines are run
in the context of different users, such as when switching between
KFP and the local orchestrators, this can create problems with
permissions.

Running the KFP containers with the same UID/GID as the local
user maintains permissions consistent across orchestrators.

Some files and folders in the local repositories (e.g. database,
.system TFX directories) are shared between different runs of the
same pipeline or of different pipelines. If pipelines are run
in the context of different users, such as when switching between
KFP and the local orchestrators, this can create problems with
permissions.

Running the KFP containers with the same UID/GID as the local
user maintains permissions consistent across orchestrators.

Signed-off-by: Stefan Nica <stefan@zenml.io>
@stefannica stefannica added the internal To filter out internal PRs and issues label Jan 10, 2022
Copy link
Contributor

@schustmi schustmi left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one question: Do you know if the bug also happened on windows in some form? Or do they just not care about any permissions and it already worked before this PR?

@stefannica
Copy link
Contributor Author

Looks good to me, just one question: Do you know if the bug also happened on windows in some form? Or do they just not care about any permissions and it already worked before this PR?

I don't think we even support KFP as a local orchestrator on Windows, but even if we did, the Windows host should ignore file permissions set on the KFP side.

Copy link
Contributor

@AlexejPenner AlexejPenner left a comment

Choose a reason for hiding this comment

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

lgtm, it might be worth checking out how it behaves on windows though

@stefannica stefannica force-pushed the stefan/eng-298-kfp-local-store branch from cb9f5d7 to ee0942d Compare January 10, 2022 15:43
@bcdurak
Copy link
Contributor

bcdurak commented Jan 10, 2022

looks good to me as well!

@stefannica stefannica merged commit b6d0fa5 into main Jan 11, 2022
@stefannica stefannica deleted the stefan/eng-298-kfp-local-store branch January 11, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal To filter out internal PRs and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants