Skip to content

Conversation

@belitskiy
Copy link
Collaborator

@belitskiy belitskiy commented Sep 16, 2024

In GH Actions context, labels do not get updated on simply being added to the PR.
They usually require a change be pushed to refresh them:
https://github.com/orgs/community/discussions/39062

In GH Actions context, labels do not get updated on simply being added,
usually requiring a change be pushed to refresh them:
https://github.com/orgs/community/discussions/39062
@google-cla
Copy link

google-cla bot commented Sep 16, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@belitskiy belitskiy marked this pull request as ready for review September 16, 2024 18:11
@belitskiy
Copy link
Collaborator Author

Merged all the steps into one, moved some more stuff into Python, PTAL

@belitskiy belitskiy changed the title Use GH API to retrieve PR labels when waiting on connection. Use GH API to retrieve PR labels when waiting on connection, use Python. Sep 18, 2024
Copy link

@MichaelHudgins MichaelHudgins left a comment

Choose a reason for hiding this comment

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

Overall looking good and along the lines of what i was wanting, thanks!

Choose a reason for hiding this comment

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

This file needs the standard copyright header and to be linted. (I think it should be picking up jax's linting standards which i am thinking we will copy over for the new repo)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. I ran the the pre-commit commands from their instructions:

   # be in jax repo root
   pip install pre-commit
   pre-commit run --all

# Pick an existing Python alias
python_bin=$(which python3 2>/dev/null || which python)
# Wait for the connection, if a halt was requested via a label/input
"$python_bin" "$GITHUB_ACTION_PATH/wait_for_connection.py"

Choose a reason for hiding this comment

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

I don't want a script failure to fail the overall job. I think there are a few ways we can go about that but the easiest might just be an || true here to keep a script failure from getting back to the job.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added continue-on-error to the step - I think it's a little clearer

Choose a reason for hiding this comment

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

Not necessary until we get the proper new repo for this (still working on it....), but i think we will want to have tests on these as much as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Acknowledged. Not adding now - not sure if I'll be fast enough - also want to focus on the connection timeout erroring out + internal doc

@belitskiy
Copy link
Collaborator Author

@MichaelHudgins PTAL

@belitskiy belitskiy removed the request for review from MichaelHudgins September 27, 2024 15:59
@belitskiy belitskiy marked this pull request as draft September 27, 2024 16:00
@belitskiy belitskiy force-pushed the use_api_labels_in_ssh branch from b2e7217 to 0667304 Compare September 27, 2024 16:07
@belitskiy belitskiy force-pushed the use_api_labels_in_ssh branch 3 times, most recently from 4ba08d5 to 69b7cf8 Compare September 27, 2024 17:00
@belitskiy belitskiy force-pushed the use_api_labels_in_ssh branch from 69b7cf8 to d43e84c Compare September 27, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants