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

added unit test to verify xla compilation of RandomRotation preprocessing layer. #17183

Closed
wants to merge 1 commit into from

Conversation

copybara-service[bot]
Copy link

added unit test to verify xla compilation of RandomRotation preprocessing layer.

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

What is the scope of this?

keras-team/keras-cv#402 (comment)

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

@qlzh727 Are we going to interact with copybara bot PRs?

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

@qlzh727
Copy link
Member

qlzh727 commented Nov 4, 2022

@divyashreepathihalli who is author of this change.

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

@divyashreepathihalli who is author of this change.

Do you know why she is not visible in the commit and masked by tensorflower-gardener bot?

@qlzh727
Copy link
Member

qlzh727 commented Nov 4, 2022

Internally we have a bot service that matches the internal user name against github author name, and populate more author information based on that. Maybe Divya didn't register that.

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

Internally we have a bot service that matches the internal user name against github author name, and populate more author information based on that. Maybe Divya didn't register that.

Yes it would be nice to have this step on-boarding OSS projects on Github.

But the historically issue of requiring to mention the commit author on this type of PRs is still there.
Can the bot at least mention the author in the description/set as Assignees?

In this way the automatically notification will work.

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

@qlzh727 This is a well known issue #15006 (comment) since 2021. I don't know how we could make some progress to better interact with the community.

@divyashreepathihalli Can you tell me something more about the context of this PR considering all our previous threads on this topic:
keras-team/keras-cv#402 (comment)
tensorflow/tensorflow#55194
tensorflow/tensorflow#55335

@qlzh727
Copy link
Member

qlzh727 commented Nov 4, 2022

I think usually the cl author will be attached to the github PR, but not for any internal reviewer. The cl author should be able to comment on the PR, and involve more internal parties if needed. The internal tool setup was one of the on-boarding steps internally, but being optional and might get skipped.

@bhack
Copy link
Contributor

bhack commented Nov 4, 2022

I think usually the cl author will be attached to the github PR, but not for any internal reviewer. The cl author should be able to comment on the PR, and involve more internal parties if needed

Without mentioning in the description or add as assignee by the bot he will never be notified on community comment. Plus, if the "github mapping" is optional we don't know how to notify the author at all.

@bhack
Copy link
Contributor

bhack commented Nov 7, 2022

@qlzh727 As you can see with last 6 months stats it is hard to interact with "anon" PRs for the community

git shortlog -s -n --since="2022-05-07" | head -n 5

   111	TensorFlower Gardener
    59	Chen Qian
    43	A. Unique TensorFlower
    43	Scott Zhu
    41	Haifeng Jin

@qlzh727
Copy link
Member

qlzh727 commented Nov 7, 2022

Ack, let me send an email to the team to make sure they have the proper mapping setup.

@bhack
Copy link
Contributor

bhack commented Nov 7, 2022

Ok, thanks. To come back to the main topic waiting for a @divyashreepathihalli's comment.

@qlzh727
Copy link
Member

qlzh727 commented Nov 7, 2022

Btw, I have asked the team to populate the internal system, and the author info should show up for the new PRs.

@bhack
Copy link
Contributor

bhack commented Nov 7, 2022

Btw, I have asked the team to populate the internal system, and the author info should show up for the new PRs.

Thanks, where we can open a ticket for https://github.com/apps/copybara-service to add the author to the Github pr description or to the Assignees list?

@qlzh727
Copy link
Member

qlzh727 commented Nov 8, 2022

I don't think there is a channel for that. I can file a request internally, since copybara is mostly a internal tool for Google.

@bhack
Copy link
Contributor

bhack commented Nov 8, 2022

If it isn't part of https://github.com/google/copybara an internal ticket would be useful thanks.

@bhack
Copy link
Contributor

bhack commented Nov 8, 2022

If it isn't part of https://github.com/google/copybara repo on Github an internal ticket would be useful thanks.

@bhack
Copy link
Contributor

bhack commented Nov 8, 2022

@qlzh727 The body seems already available in the API so it is just a configuration of the app https://github.com/google/copybara/blob/master/docs/reference.md#gitgithub_pr_destination

@qlzh727
Copy link
Member

qlzh727 commented Nov 8, 2022

@bhack, the internal config is a templated service, and didn't expose the github_pr_destination to us. Let me add @mikelalcon from copybara team for more inputs.

@bhack
Copy link
Contributor

bhack commented Nov 8, 2022

@qlzh727 Thanks, can we move this part of the topic in a new place if we want to keep it public cause If not it will be hard to follow the original topic of this PR.

@divyashreepathihalli
Copy link
Collaborator

Ok, thanks. To come back to the main topic waiting for a @divyashreepathihalli's comment.

Hey @bhack, sorry about the late response. I do not intend to submit this PR until I can enable ImageProjectiveTransform to be XLA compilable. I am currently collecting data on layer behaviors when XLA compiled. You can ignore this PR for now.

@bhack
Copy link
Contributor

bhack commented Nov 11, 2022

Ok thanks, as this is a historical well known not covered OPS by XLA and we have many related tickets to track this and a partial MLIR bridge PR.

@divyashreepathihalli
Copy link
Collaborator

okay thanks!

@bhack
Copy link
Contributor

bhack commented Nov 11, 2022

layer behaviors when XLA compiled

More in general for this you could be interested also in:
tensorflow/tensorflow#56510
tensorflow/tensorflow#57923

@sachinprasadhs sachinprasadhs deleted the test_483770485 branch February 1, 2024 22:38
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.

None yet

5 participants