-
Notifications
You must be signed in to change notification settings - Fork 861
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
Tabular: Add available disk logging + warning #3069
Conversation
disk_stats = ResourceManager.get_disk_usage(path=self.path) | ||
disk_free_gb = disk_stats.free / 1e9 | ||
disk_total_gb = disk_stats.total / 1e9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice you created class methods for get_disk_usage_free
, get_disk_usage_used
, and get_disk_usage_total
but just used get_disk_usage
and extracted the free
and total
values. Should we consider deleting the get_disk_usage_{free,used,total}
methods if we aren't going to use them, it's easy to extract the members individually, and will avoid multiple calls to shutil.disk_usage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I think they aren't necessary, so I've removed them.
logger.log(disk_verbosity, | ||
f'Disk Space Avail: {disk_free_gb:.2f} GB / {disk_total_gb:.2f} GB ' | ||
f'({disk_proportion_avail * 100:.1f}%){disk_log_extra}') | ||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its best to catch as specific of an exception as possible. This would allow us to make the following log statement if the issue is indeed related to the disk_usage
call, but if another error is raised in this try block, we don't accidentally hide if from the user. If you know of a more specific exception, it could be good to use it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unclear what exception it would raise, as it would require me being able to simulate some strange unknown environment where this logic doesn't work correctly, and the shutil code does not make it clear what exception would be raised in these strange situations. Because of this, it is safest to have a generic exception catch to avoid completely breaking AutoGluon in these more niche scenarios that I don't know about. I'm unsure if these scenarios even exist, but it is very hard to rule out the possibility. (For example, running in the browser, running in colab, running in kaggle, running in docker, running on windows, running on specialized private hardware, running in a fully-in-memory disk environment, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in-line comment explaining this in the code
Job PR-3069-88242ab is done. |
Job PR-3069-35fb649 is done. |
Issue #, if available:
Description of changes:
Example:
Low Available Disk:
Standard (sufficient disk space):
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.