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.

Sign up for GitHub

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

Jump to bottom

Fix mount permissions on input volumes #492

Merged
merged 57 commits into from
Dec 1, 2023
Merged

Fix mount permissions on input volumes #492

merged 57 commits into from
Dec 1, 2023

Conversation

aristizabal95
Copy link
Contributor

@aristizabal95 aristizabal95 commented Oct 6, 2023 •
edited by hasan7n
Loading

This PR fixes and simplifies ensuring read-only access to input volumes on MLCube execution. This is done so by leveraging on the recent --mount=ro flag, instead of specifying per-volume permissions
Closes #488

@aristizabal95
Add Data Preparator cookiecutter template
28f7e7f
@aristizabal95
Rename cookiecutter folder
6f9e19e
@aristizabal95
Temporarily remove possibly offending files
df6e6a2
@aristizabal95
Remove cookicutter conditionals
a7db0cf
@aristizabal95
Inclube back missing pieces of template
a7a6d15
@aristizabal95
remove cookiecutter typo
e2f7108
@aristizabal95
Use project_name attribute
581b5bb
@aristizabal95
Change cookiecutter fields order
fd77804
@aristizabal95
Create empty directories on hook
6eebd59
@aristizabal95
Fix empty folders paths
5ef86a2
@aristizabal95
Create evaluator mlcube cookiecutter template
d04baf8
@aristizabal95
Fix JSON Syntax Error
02cec01
@aristizabal95
Update template default values
b3d7a1d
@aristizabal95
Remove reference to undefined template variable
7338236
@aristizabal95
Implement model mlcube cookiecutter template
d1cec5e
@aristizabal95
Update cookiecutter variable default values
7338755
@aristizabal95
Create medperf CLI command for creating MLCubes
3ae9226
@aristizabal95
Provide additional options for mlcube create
e07cde2
@aristizabal95
Start working on tests
68e136a
@aristizabal95
Add tests for cube create
b8e03ac
@aristizabal95
Ignore invalid syntax on cookiecutter conditionals
7896b25
@aristizabal95
Ignore more flake8 errors
4f78981
@aristizabal95
Remove unused import
f5dab5e
@aristizabal95
Empty commit for cloudbuild
a03d7f6
@aristizabal95
Fix inconsistency with labels paths
6bb60d0
@aristizabal95
Update mlcube.yaml so it can be commented on docs
43b6cab
@aristizabal95
Don't render noqa comments on template
55b5d22
@aristizabal95
Remove flake8 specific ignores
135c598
@aristizabal95
Exclude templates from lint checks
e9e2c32
@aristizabal95
Remove specific flake8 ignores
e95dab8
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
69ae2ed
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
8a47ad9
@aristizabal95
Abstract field-error dict formatting
5f32e50
@aristizabal95
Reformat errors dictionary for printing
3951af8
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
09b8d69
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
e7a8ae4
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
091b69c
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
c69bf4c
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
d0c2b77
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
c70bb9b
@aristizabal95
Merge branch 'main' of https://github.com/mlcommons/medperf
90b5cb3
@aristizabal95
Implement general read-only input permission
36cdab2
@aristizabal95
Remove unnecessary permission definition
8ef2bc7
@aristizabal95
Fix/implement ro tests
f7b4959
@aristizabal95 aristizabal95 added type: bug Something isn't working component: client issues regarding the CLI labels Oct 6, 2023
@aristizabal95 aristizabal95 self-assigned this Oct 6, 2023
@aristizabal95 aristizabal95 requested a review from a team as a code owner October 6, 2023 20:27
@aristizabal95 aristizabal95 temporarily deployed to testing-external-code October 6, 2023 20:27 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023 •
edited
Loading

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

msheller
msheller reviewed Oct 13, 2023
View reviewed changes
Copy link

@msheller msheller left a comment

Choose a reason for hiding this comment

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

Two questions:

  1. I think I am missing something. It looks to me like "*_str_params" isn't used in the run function in the old code. If so, wouldn't the fix have been to use them? Or was that variable intending to use a feature in MLCube that is broken/missing?

  2. Am I correct that MLCube won't set 'ro' for the output directories, even though you've specified --mount=ro? If so, I wonder if we should request a name change on that parameter, as it's confusing to me to see "--mount" and it not apply to all mount points.

@hasan7n hasan7n self-requested a review November 27, 2023 17:32
hasan7n
hasan7n approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@hasan7n hasan7n 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!

@hasan7n
Copy link
Contributor

hasan7n commented Nov 27, 2023

@msheller

  1. It was used. The first line of the cube.run was updating the kwargs with the string parameters. The cause of the bug is described here: [BUG] Data folders are not mounted as read-only as expected #488
  2. Yes, the --mount will only act on the inputs. I will open an issue for name change in the mlcube repo.

@hasan7n hasan7n had a problem deploying to testing-external-code December 1, 2023 11:12 — with GitHub Actions Failure
@hasan7n hasan7n merged commit de0ec67 into mlcommons:main Dec 1, 2023
6 of 7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@msheller msheller msheller left review comments

@hasan7n hasan7n hasan7n approved these changes

Assignees

@aristizabal95 aristizabal95

Labels
component: client issues regarding the CLI type: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Data folders are not mounted as read-only as expected
3 participants
@aristizabal95 @hasan7n @msheller