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

MT-60: Add a step to check the disk-quota in mila code #53

Merged
merged 5 commits into from
Sep 21, 2023

Conversation

lebrice
Copy link
Collaborator

@lebrice lebrice commented Aug 31, 2023

Checks the disk usage before allocating a job.

  • If the disk usage or # of files is above the limit, raises an error
  • If the usage is > 90% of the limit, prints a warning with some instructions and a reference to the documentation.

@lebrice lebrice marked this pull request as ready for review August 31, 2023 19:53
Signed-off-by: Fabrice Normandin <[email protected]>
Copy link
Member

@satyaog satyaog left a comment

Choose a reason for hiding this comment

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

It would be nice to have this partly tested but I think we'll soon do another round to add tests anyway

max_files,
) = last_line_parts.split("|")
used_gb = float(used_gb.removesuffix("GiB").strip())
max_gb = float(max_gb.removesuffix("GiB").strip())
Copy link
Member

Choose a reason for hiding this comment

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

str.removesuffix() has been added in python 3.9 I believe which is higher than the minimum stated in the pyproject.toml. Are we ok with bumping the minimal version to 3.9? I personally don't have any objections as mila tools is not likely to become a python packages dependency in any project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 1b6a509

@lebrice
Copy link
Collaborator Author

lebrice commented Sep 18, 2023

It would be nice to have this partly tested but I think we'll soon do another round to add tests anyway

Oops, I did add some unit tests for this, I believe I just forgot to push the commits. Will fix this tomorrow.

Turns out I don't have a test for this, I got mixed up with #54

Signed-off-by: Fabrice Normandin <[email protected]>
(not sure why or how it got removed in the first place tbh)

Signed-off-by: Fabrice Normandin <[email protected]>
@satyaog
Copy link
Member

satyaog commented Sep 21, 2023

Merging, we'll add tests later

@satyaog satyaog merged commit 9b3dabb into mila-iqia:master Sep 21, 2023
5 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
Development

Successfully merging this pull request may close these issues.

2 participants