Add regression tests for get_latest and get_oldest functions #110
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code was calling `os.path.join()` too many times and causing the path to be repeated unnecessarily. Signed-off-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
weiji14
force-pushed
the
repeated-ckpt-path
branch
from
3d2364c to
d0919fa
Compare
nairbv
reviewed
Aug 26, 2024
| ], | ||
| key=lambda path: int(path.split("/")[-1].split("_")[1]), | ||
| [x for x in os.listdir(targdir) if qualifier(os.path.join(targdir, x))], | ||
| key=lambda path: int(path.split("_")[1]), |
Contributor
There was a problem hiding this comment.
probably the same issue below with get_oldest ?
maybe we could add a unit test?
Author
Contributor
There was a problem hiding this comment.
looks better, though now the ctime key fails in get_oldest
Author
There was a problem hiding this comment.
Ok, have pushed a fix, can you run the tests again to see if it works?
Signed-off-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
When a list of 3 files are passed into the get_oldest and get_latest functions, ensure that the files that were created first and last are returned respectively. Signed-off-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
weiji14
force-pushed
the
repeated-ckpt-path
branch
from
20c95a4 to
f082164
Compare
Signed-off-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
weiji14
force-pushed
the
repeated-ckpt-path
branch
from
2c843ba to
cf39fc7
Compare
Merged
daviswer
added a commit
that referenced
this pull request
Oct 11, 2024
Updates `get_latest` and `get_oldest` to use the same sorting function, and allows the dataloader ckp handler to pass in its custom sort manually. Removes the bug where excessive path joins lead to repeated path prefixes in dataloader ckp loading. Fixes GPTBigCode signatures used for speculator training, to match superclass signatures (currently preventing other PRs from landing). Includes and subsumes #110 and #96. Full credit to @weiji14 and @Akash-Nayak respectively
weiji14
changed the title
Author
|
Bug fix has been applied in #119, so this PR only add unit tests now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Edit: Bug fix was applied in #119. This PR thus only adds two unit tests to prevent bug regression.
Code was calling
os.path.join()too many times and causing the path to be repeated unnecessarily, resulting in errors like:This patch removes one of the
os.path.join()calls, and also thepath.split("/")[-1]part which was needed because a path with "/" dividers was used in the list comprehension.