Skip to content
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

Implement clone() #429

Merged
merged 9 commits into from
May 25, 2024
Merged

Implement clone() #429

merged 9 commits into from
May 25, 2024

Conversation

tfogal
Copy link
Collaborator

@tfogal tfogal commented May 17, 2024

Fixes #342.

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes #342

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Always ;-)

@tfogal
Copy link
Collaborator Author

tfogal commented May 17, 2024

The failures are distributed notebook flakiness; unrelated, so marking ready for review.

I didn't understand the #342 comment about not making this a symbol. Without the @torchsymbol decorator, clone calls won't map to this implementation, of course. Maybe someone could help educate me on how else this could be done?

@tfogal tfogal marked this pull request as ready for review May 17, 2024 16:58
@tfogal
Copy link
Collaborator Author

tfogal commented May 17, 2024

The test failures are for distributed things:
https://dev.azure.com/Lightning-AI/lightning/_build/results?buildId=202766&view=logs&j=b97dbf6d-98bd-5b68-7c01-878b39c3da28&t=3c72ede2-92c1-5cd2-2bac-ad2411af2aea&l=306
which seem unrelated. Let's try merging main into this...

@mruberry
Copy link
Collaborator

triage review — just return a for the impl seems OK for now — we can follow-up if that confuses practitioners or we want more clarity

@tfogal
Copy link
Collaborator Author

tfogal commented May 20, 2024

The failures are just the distributed tests failing, issue #432. This is ready for re-review / merging.

@tfogal
Copy link
Collaborator Author

tfogal commented May 24, 2024

poke @mruberry @t-vi for review/merging

@tfogal tfogal mentioned this pull request May 25, 2024
4 tasks
tfogal and others added 2 commits May 25, 2024 14:10
Since we're incorrectly returning an alias, modifications to the
returned tensor will show up in the original tensor. Add a test
that demonstrates this, which should remind us to revisit this
implementation once we properly implement functionalization.

Thanks Luca Antiga for the idea.

Also, it turns out PyTorch fixed an issue with their sparse_coo in
the interim and so that test started passing. So this also removes
the sparse test's xfail.
Copy link
Collaborator

@lantiga lantiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

@lantiga lantiga merged commit 7d6e540 into main May 25, 2024
37 checks passed
@lantiga lantiga deleted the tfogal/clone branch May 25, 2024 15:19
crcrpar pushed a commit that referenced this pull request May 29, 2024
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement TensorBase.clone
4 participants