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

Allow PR refs for jax or xla #116

Closed
wants to merge 6 commits into from
Closed

Allow PR refs for jax or xla #116

wants to merge 6 commits into from

Conversation

terrykong
Copy link
Contributor

We already allow standard git-refs to be configurable in the build for xla and jax.

This change makes it possible to specify a PR git-ref so we can test an experimental change.

@terrykong terrykong requested a review from yhtang July 12, 2023 17:52
@yhtang
Copy link
Collaborator

yhtang commented Jul 17, 2023

Could you please provide an example of how PRs are specified for a build?

@terrykong
Copy link
Contributor Author

Sure. So for this PR: https://github.com/openxla/xla/pull/4044/files

You'd specify it in the CI workflow as https://github.com/openxla/xla.git#pull/4044/head

@yhtang
Copy link
Collaborator

yhtang commented Jul 18, 2023

We can make it more intuitive. pull/<ID>/heads feels a bit magical here, and users would have to guess the correct format from the git refs.

Given that a PR is referenced in GitHub Flavored Markdown by user/repo#<PR_ID>, which coincides with that ci.yaml currently accepts, how about we determine if the content after # is a PR if it is:

  • purely numeric
  • decimal

There is a small chance of conflict if a branch is named by a decimal integer, which is extremely rare in practice. The chance that it collides with a commit hash is also negligible.

Moreover, this job might be a better place to host such logic. The idea is to keep the Dockerfile clean and independent of the CI workflow. We can detect if the content after # is a PR, then convert it to the corresponding git ref and pass it into subsequent reusable workflows/Dockerfiles.

@terrykong terrykong self-assigned this Jul 18, 2023
@terrykong
Copy link
Contributor Author

GitHub Flavored Markdown by user/repo#<PR_ID>

That's cool! I didn't realize that was that syntax in GH markdown.

Moreover, this job might be a better place to host such logic. The idea is to keep the Dockerfile clean and independent of the CI workflow. We can detect if the content after # is a PR, then convert it to the corresponding git ref and pass it into subsequent reusable workflows/Dockerfiles.

We can definitely move the logic that converts integers to PR git-refs 1320 -> pull/1320/head in that job. But, I'm not sure of a better way to reference the PR other than what's already in the Dockerfile. It is a bit magical that that's the git-ref, but it is what is documented in the GH's docs

I can make the change to accept a decimal instead.

@terrykong
Copy link
Contributor Author

Closing this as it complicates the dockerfile. Better ways of handling this were discussed offline like potentially moving the logic of fetching github git refs outside of the dockerfile

@terrykong terrykong closed this Jul 28, 2023
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