-
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
Disable additional transforms for the PyTorch Autograd path #74
Merged
IvanYashchuk
merged 7 commits into
Lightning-AI:main
from
IvanYashchuk:disable-additional-transforms
Mar 26, 2024
Merged
Disable additional transforms for the PyTorch Autograd path #74
IvanYashchuk
merged 7 commits into
Lightning-AI:main
from
IvanYashchuk:disable-additional-transforms
Mar 26, 2024
Conversation
This file contains 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
IvanYashchuk
requested review from
mruberry,
lantiga,
robieta,
t-vi and
carmocca
as code owners
March 25, 2024 16:07
t-vi
approved these changes
Mar 25, 2024
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.
Thank you @IvanYashchuk
So I generally agree with the patch, but the CI has opinions. |
nikitaved
reviewed
Mar 25, 2024
Notebooks fail with:
|
Next error in the notebooks: nbclient.exceptions.CellExecutionError: An error occurred while executing the following cell:
------------------
### DON'T TRY THIS AT HOME
computation_trace.bound_symbols[2].sym = cache_rec.computation_traces[0].bound_symbols[2].subsymbols[0].sym
if cache_rec.computation_traces[0].bound_symbols[3].subsymbols:
computation_trace.bound_symbols[3] = cache_rec.computation_traces[0].bound_symbols[3].subsymbols[0]
computation_trace.bound_symbols[4].sym = cache_rec.computation_traces[0].bound_symbols[4].subsymbols[0].sym
wrap_as_highlighted_code(computation_trace) |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Applying transforms on the trace after "forward backward split + transform_for_execution" is not a well-tested path and breaks the careful sorting of operations introduced inside the
split_forward_backward
function. In the future, we need to revisit the order of transformations and move execution transforms out of thesplit_forward_backward
function. Today this PR is needed to restore distributed communication overlap with computation and unblock fixing TransformerEngine+DataParallel.