[WIP] Use file metadata to determine whether profiler config should be reloaded. #463
Conversation
Codecov Report
@@ Coverage Diff @@
## master #463 +/- ##
===========================================
- Coverage 65.62% 50.13% -15.50%
===========================================
Files 172 162 -10
Lines 13260 12919 -341
===========================================
- Hits 8702 6477 -2225
- Misses 4558 6442 +1884
Continue to review full report at Codecov.
|
| assert case_insensitive_profiler_config_parser.config.detailed_profiling_config.num_steps == 3 | ||
|
|
||
|
|
||
| def test_update_step_profiler_config_parser( |
There was a problem hiding this comment.
Can you run this test inside of a loop, to ensure that we do not have flaky behavior.
There was a problem hiding this comment.
Would be better to trivially use pytest.mark.parametrize so that the test itself doesn't change, but we simply run it multiple times and ensure it passes each time.
How many times do you suggest the test be run? 3?
There was a problem hiding this comment.
Also, first and foremost, can you explain why this behavior might be flaky? The workflow seems to be clear cut:
- Set up the profiler config JSON
- Set up the profiler config parser object (which automatically loads the config)
- Load the config again
- Verify the last modified time has not changed
- Replace the config JSON
- Load the config again
- Verify the last modified time has changed
TL;DR the calls to load_config are controlled, so we know exactly the JSON that is at the path before loading the config each time.
smdebug/profiler/utils.py
Outdated
| """ | ||
| Get the last time that the file at the given filepath was modified, in the form of a datetime object. | ||
| """ | ||
| last_modified_time = Path(filepath).stat().st_mtime |
There was a problem hiding this comment.
Can you explain in comments, the reasons for the choice of this implementation?
A simple link to documentation might also suffice.
https://docs.python.org/3/library/stat.html#stat.ST_MTIME
There was a problem hiding this comment.
The advantage of relying on file metadata to determine whether a profiler config should be reloaded is stated in the PR description - we cut down on the amount of times the JSON needs to be loaded into memory.
The choice of specifically using pathlib to get the file metadata was arbitrary - we can just as easily use os with os.path.getmtime(path). In fact, I probably will switch to os.path.getmtime(path) since it appears cleaner.
|
|
||
| # check that reloading the config when it has changed will update the config fields. | ||
| monkeypatch.setenv("SMPROFILER_CONFIG_PATH", new_step_profiler_config_parser_path) | ||
| shutil.copy(new_step_profiler_config_parser_path, step_profiler_config_parser_path) |
There was a problem hiding this comment.
Tests are currently failing since this call has different behavior for Linux and Unix (for Linux, this copies the file metadata too).
Will need to fix this by finding a better API for copying files or writing the JSON manually.
ndodda-amazon
changed the title
|
Closing for now since the root cause of the CI failures has to do with Linux flakiness in the metadata of the profiler config JSON not being updated. Will reopen once I've fixed this issue. |
3 participants
Description of changes:
For each step, we need to determine if the profiler config JSON has changed, and if so, we should reload the profiler config. Currently, we reload the JSON into memory and physically check whether the file contents have changed in order to determine if the profiler config should be reloaded. However, this may pose problems for performance at scale because we would be loading a JSON object into memory at each step.
This change replaces the above check by inspecting the file metadata for the last modified time. If the last modified time has changed, that means the file has changed and we should reload the profiler config. This is done without loading the JSON into memory (see tests, which verify that the config file is not accessed (read into memory) if the file has not been modified).
Style and formatting:
I have run
pre-commit installto ensure that auto-formatting happens with every commit.Issue number, if available
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.