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

[PJRT:GPU] Fail on unknown memory space for execution output buffers #14328

Closed

Conversation

jaro-sevcik
Copy link
Contributor

This PR addresses @yliu120 's post-landing comments from #14088:

  1. Error out on unknown memory_space in the output's layout (this requires a bit of plumbing to propagate the error).
  2. Use memory_space_by_kind_id in place of memory_space_by_kind (this requires some ugly cast, not sure if it's worth it - performance should not matter much here).
  3. Avoid shape copy, pass by reference.

Copy link
Member

@dimitar-asenov dimitar-asenov left a comment

Choose a reason for hiding this comment

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

This changes here are fine, but there are no test changes. While the changes are small, they do include API changes, and I'm a bit surprised that no test needs to change. Could you please add a test?

@jaro-sevcik
Copy link
Contributor Author

This changes here are fine, but there are no test changes. While the changes are small, they do include API changes, and I'm a bit surprised that no test needs to change. Could you please add a test?

This error path is not easy to test because you would need to create an executable with output memory space that are not present on device. I think this might only be possible by hacking serialized executable. Is that what you have in mind?

As for the API change, this is a private method that is not used anywhere else (but perhaps it is used/overridden in PjRtTpuClient or InternalPjRtTpuClient?).

@yliu120 , could you please clarify what you had in mind in #14088 (comment)?

@dimitar-asenov
Copy link
Member

OK, I see that the API is actually really an internal thing, and this change covers all the uses.

copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Jul 4, 2024
…ut buffers

Imported from GitHub PR openxla/xla#14328

This PR addresses @yliu120 's post-landing comments from openxla/xla#14088:

 1. Error out on unknown memory_space in the output's layout (this requires a bit of plumbing to propagate the error).
 2. Use `memory_space_by_kind_id` in place of `memory_space_by_kind` (this requires some ugly cast, not sure if it's worth it - performance should not matter much here).
 3. Avoid shape copy, pass by reference.
Copybara import of the project:

--
b64a7f97e3cc080f3bba43ceac0f534467f1f2a6 by Jaroslav Sevcik <[email protected]>:

Address reviewer comments

Merging this change closes #14328

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#14328 from jaro-sevcik:clean-up-after-pr-14088 b64a7f97e3cc080f3bba43ceac0f534467f1f2a6
PiperOrigin-RevId: 649324323
@copybara-service copybara-service bot closed this in c2d2bdd Jul 4, 2024
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Jul 4, 2024
…ut buffers

Imported from GitHub PR openxla/xla#14328

This PR addresses @yliu120 's post-landing comments from openxla/xla#14088:

 1. Error out on unknown memory_space in the output's layout (this requires a bit of plumbing to propagate the error).
 2. Use `memory_space_by_kind_id` in place of `memory_space_by_kind` (this requires some ugly cast, not sure if it's worth it - performance should not matter much here).
 3. Avoid shape copy, pass by reference.
Copybara import of the project:

--
b64a7f97e3cc080f3bba43ceac0f534467f1f2a6 by Jaroslav Sevcik <[email protected]>:

Address reviewer comments

Merging this change closes #14328

PiperOrigin-RevId: 649341102
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.

2 participants