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

[RFC] Moving TSL into XLA #100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[RFC] Moving TSL into XLA #100

wants to merge 1 commit into from

Conversation

ddunl
Copy link
Member

@ddunl ddunl commented Dec 12, 2023

RFC about moving TSL into XLA. Please leave comments by Jan 12th!

@ddunl ddunl changed the title Add moving TSL into XLA RFC [RFC] Moving TSL into XLA Dec 12, 2023
@i-chaochen
Copy link

IIUC, tsl would move from xla/third_party/tsl to xla/tsl ?

@ddunl
Copy link
Member Author

ddunl commented Feb 14, 2024

That's correct, that should be the only change inside the XLA repo

@ManfeiBai
Copy link

Hi, @ddunl, thanks for sharing this and here are some questions after reading the proposal:

  1. would this be a one/several PR change or a long list of PRs' change?
  • Currently, PyTorch/XLA update OpenXLA-pin monthly, and it seems PyTorch/XLA would include this change(@tsl to @xla/tsl) within one or several OpenXLA-pin updates;
  • To avoid crash&revert, could has a list of PyTorch/XLA PRs include per change to run CI tests before merge
  1. does TSL function interface be affected after the moving?

cc @miladm

@ddunl
Copy link
Member Author

ddunl commented Feb 14, 2024

Hi Manfei,

  1. This will be several changes (probably around 40 or so)
  2. I should be able to do it in such a way that everything happens on a single pin update. PTXLA doesn't use TSL too much directly: https://github.com/search?q=repo%3Apytorch%2Fxla+%40tsl%2F%2F&type=code , and I was already planning on doing the tsl/platform code last as it is the most widely used.
  3. Only the moves of the targets in the above search will be affected, so I'll make sure that you are notified as those are submitted.
  4. None of the code itself should change, including symbol names (i.e. everything will still be in namespace tsl. This was a big problem moving to TSL, so I'm going to save the trouble this time and only move files and change target locations, not any of the code itself.

that other projects might want to move out of TensorFlow. In this case, TSL would provide common support code for
potentially many projects, not just XLA.
However, since TSL's creation over a year ago, XLA is the only direcet dependent of TSL, so we are in the process
of planning to move to TSL into XLA under the `tsl/` subdirectory.
Copy link
Contributor

@joker-eph joker-eph Feb 14, 2024

Choose a reason for hiding this comment

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

Nit: you're providing a good history here, but the pros/cons of doing this work aren't really stated here: can you provide some insights into this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good point, I should be more explicit: it's mostly that the Copybara configurations are significantly more complex than they otherwise would be due to vendoring, so since all TSL users are also XLA users (or XLA itself), it lets us cut down on the complexity and reduce maintenance burden. I think for users of openxla/xla there is really no concrete benefit or drawback (or the benefits/drawbacks are small in magnitude), but the reduction in maintenance burden on the google side is significant. It also lets us make progress on things like the move from tensorflow/compiler/xla to third_party/xla internally.

The above logic informs the plan - given that there were no concerns raised about moving forward, the hope is that by moving it into XLA but still keeping TSL 'higher' in the dependency graph, we can let other projects who might want TSL separate from XLA in the future have it without bloated binary sizes but also do the cleanups that we want internally.

I should've elaborated more clearly on this from the start - let me know if this comment makes it clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for elaborating :)

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.

4 participants