-
Notifications
You must be signed in to change notification settings - Fork 87
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
fix crash in data loader caused by using stale array #765
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.
LGTM - just left a few notes on comments I found confusing if I imagined reading them without context.
@@ -98,6 +99,8 @@ def __iter__(self): | |||
return self.iter_from_step(None) | |||
|
|||
def iter_from_step(self, start_from_batch: Optional[int] = None): | |||
# sometimes we pass in an array for the start_from_batch, so we need to check for that | |||
start_from_batch = int(start_from_batch) if start_from_batch is not None else None |
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.
Is there a way to account for the case the comment describes in the MyPy type hint?
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 could make it be an jnp.ndarray, but i'd rather we didn't pass it in and i'm vaguely annoyed mypy isn't raising an error already.
src/levanter/data/loader.py
Outdated
|
||
gda_tree = jax.tree.unflatten(self.dl._ex_structure, gda_leaves) | ||
yield gda_tree | ||
# TODO: this doesn't work with named axes |
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.
Wouldn't anything with named axes go into the if isinstance(leaf_data, hax.NamedArray)
above rather than the parent else
?
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.
yeah old comment, sorry
The error in the TPU test doesn't look like a flaky one to me though! |
yeah it's not flaky. it's real. I might merge anyway though b/c it's a low priority test. trying to figure it out |
figured it out |
No description provided.