-
Notifications
You must be signed in to change notification settings - Fork 536
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
[Storage] Show logs for storage mount #4387
[Storage] Show logs for storage mount #4387
Conversation
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.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (3)
sky/data/storage.py:1002
- The variable 'self.source' should be 'source' to match the function parameter.
logger.warning(f'\'.git\' directory under \'{self.source}\' ' 'is excluded during sync.')
sky/data/data_utils.py:541
- The error message has a grammatical error. It should be 'No file uploaded. This could be due to an error or because all files already exist on the cloud.'
storage_logger.info('No file uploaded, could be error happened or all files already exist on cloud')
sky/data/data_utils.py:531
- [nitpick] The variable name 'stdout_line_write_cnt' is not very descriptive. Consider renaming it to 'stdout_line_count'.
stdout_line_write_cnt = 0
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.
Thanks @zpoint!
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.
Thanks @zpoint! Works nicely. Should be good to go after the two comments are resolved.
sky/sky_logging.py
Outdated
|
||
def generate_tmp_logging_file_path(file_name: str) -> str: | ||
"""Generate an absolute path of a tmp file for logging.""" | ||
run_timestamp = datetime.now().strftime('%Y-%m-%d-%H-%M-%S-%f') |
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.
We should use backend_utils.get_run_timestamp()
to have paths consistent with provisioning logs.
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 made some modifications to avoid a circular import.
@romilbhardwaj Review reminder 😊 |
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.
Thanks @zpoint! One minor comment, should be good to go.
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.
Thanks @zpoint, lgtm!
Feature for #4105
Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
conda deactivate; bash -i tests/backward_compatibility_tests.sh