-
Notifications
You must be signed in to change notification settings - Fork 12
[5/6 Deps Update] Update all workflows and actions to use UV #400
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
[5/6 Deps Update] Update all workflows and actions to use UV #400
Conversation
kmontemayor2-sc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW the name should be
[5/6 Deps Update] Update all workflows and actions to use UV right?
| - name: Checkout PR Branch (on-dispatch) | ||
| if: ${{ github.event_name == 'workflow_dispatch' }} | ||
| uses: snapchat/gigl/.github/actions/checkout-pr-branch@main | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| pr_number: ${{ inputs.pr_number }} | ||
| - name: Checkout repository (on-push) | ||
| if: ${{ github.event_name == 'push' }} | ||
| uses: actions/checkout@v4 | ||
| - name: Setup Machine for building Docker images | ||
| uses: snapchat/gigl/.github/actions/setup-python-tools@main | ||
| uses: ./.github/actions/setup-python-tools | ||
| with: | ||
| setup_gcloud: "true" | ||
| try_cleaning_disk_space: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to keep the two branches here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just introducing so this is easier to test in future; but I can remove if you prefer - I don't have strong feelings, just more work to test it as these will have to be re-introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when do we run build-cuda-base-image and friends on-push?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whenever u make changes to the rule and want to test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove it tbh if its easier ; seems its also confusing @yliu2-sc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do these prior to merge this PR btw; keeping them around to make testing easier for now.
cc @yliu2-sc
.github/workflows/on-pr-comment.yml
Outdated
|
|
||
| unit-test-python: | ||
| if: ${{ github.event.issue.pull_request && (contains(github.event.comment.body, '/unit_test_py') || endsWith(github.event.comment.body, '/unit_test')) }} | ||
| unit-test: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert these changes?
| # Uncomment strictly for testing purposes | ||
| # push: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is incomplete and shouldn't be here?
.github/workflows/on-pr-merge.yml
Outdated
| # Once before it gets into the merge queue and once when it is in the merge queue. | ||
| # Our tests take a long time to run, so this is not ideal. | ||
| if: github.event_name == 'merge_group' | ||
| # if: github.event_name == 'merge_group' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove if not needed
.github/workflows/on-pr-merge.yml
Outdated
| # Once before it gets into the merge queue and once when it is in the merge queue. | ||
| # Our tests take a long time to run, so this is not ideal. | ||
| if: github.event_name == 'merge_group' | ||
| # if: github.event_name == 'merge_group' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove if not needed
| build-cpu-base-image: | ||
| runs-on: ubuntu-latest | ||
| # runs-on: gigl-large-instances # x64 Ubuntu:latest w/ 8-cores, 32GB RAM, 300 GB SSD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was running into issues building cuda image w/ updates - it was inflating right to 16GB of context space to build the images so it would cause failures > 50% of the time. Thus I updated the machines for gpu in the backend.
Subsequently, my investigation also yielded we dont need the bigger instances for building cpu and dataflow images so i downgraded them to use default instances.
| runs-on: ubuntu-latest | ||
| # runs-on: gigl-large-instances # x64 Ubuntu:latest w/ 8-cores, 32GB RAM, 300 GB SSD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto on machine change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can discuss here: https://github.com/Snapchat/GiGL/pull/400/files#r2600213552
| - build-cpu-base-image | ||
| - build-dataflow-base-image | ||
| - build-builder-image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we need this change now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The older version of building Dataflow base image does not work anymore i.e. COPY --from=apache/beam_python3.9_sdk:2.53.0 /opt/apache/beam /opt/apache/beam with the updated python and sdk version creates runtime failures when running dataflow jobs. Thus, we are now just using apache/beam_python3.11_sdk:2.56.0 as base image instead of trying to copy from it.
Since we have updated building the docker image - it made sense to update this too.
| tests: | ||
| cora_nalp_test: | ||
| task_config_uri: "gigl/src/mocking/configs/e2e_node_anchor_based_link_prediction_template_gbml_config.yaml" | ||
| task_config_uri: "python/gigl/src/mocking/configs/e2e_node_anchor_based_link_prediction_template_gbml_config.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this change happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the root directory has now changed to be root directory not gigl directory - as per pyproject.toml moving and all as well.
This should hopefully simplify all future path resolutions without worrying what dir you are in, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I'm surprised this was the only place that this caused issues! Well I guess we got lucky :)
| - name: Checkout PR Branch (on-dispatch) | ||
| if: ${{ github.event_name == 'workflow_dispatch' }} | ||
| uses: snapchat/gigl/.github/actions/checkout-pr-branch@main | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| pr_number: ${{ inputs.pr_number }} | ||
| - name: Checkout repository (on-push) | ||
| if: ${{ github.event_name == 'push' }} | ||
| uses: actions/checkout@v4 | ||
| - name: Setup Machine for building Docker images | ||
| uses: snapchat/gigl/.github/actions/setup-python-tools@main | ||
| uses: ./.github/actions/setup-python-tools | ||
| with: | ||
| setup_gcloud: "true" | ||
| try_cleaning_disk_space: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when do we run build-cuda-base-image and friends on-push?
| pr_number: ${{ inputs.pr_number }} | ||
| - name: Setup Machine for building Docker images | ||
| uses: snapchat/gigl/.github/actions/setup-python-tools@main | ||
| uses: ./.github/actions/setup-python-tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are planning on reverting this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wont merge if I revert since it needs updated workflows to run and those workflows wont be in main when this is running CI.
Maybe post merging everything I can raise a PR.
| tests: | ||
| cora_nalp_test: | ||
| task_config_uri: "gigl/src/mocking/configs/e2e_node_anchor_based_link_prediction_template_gbml_config.yaml" | ||
| task_config_uri: "python/gigl/src/mocking/configs/e2e_node_anchor_based_link_prediction_template_gbml_config.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I'm surprised this was the only place that this caused issues! Well I guess we got lucky :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. this change should be in a different pr right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ye.... whoops.
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Background Context: #405
Some guidance on stack of PRs: #405 (comment)
Scope of PR
uv