[5/6 Deps Update] Update all workflows and actions to use UV#400
[5/6 Deps Update] Update all workflows and actions to use UV#400svij-sc merged 35 commits intosvij/update-docker-images-use-uvfrom
Conversation
kmontemayor2-sc
left a comment
There was a problem hiding this comment.
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.
do we need to keep the two branches here?
There was a problem hiding this comment.
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.
when do we run build-cuda-base-image and friends on-push?
There was a problem hiding this comment.
whenever u make changes to the rule and want to test it.
There was a problem hiding this comment.
I can remove it tbh if its easier ; seems its also confusing @yliu2-sc
There was a problem hiding this comment.
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.
revert these changes?
| # Uncomment strictly for testing purposes | ||
| # push: |
There was a problem hiding this comment.
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' |
.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' |
| 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.
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.
ditto on machine change.
There was a problem hiding this comment.
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.
any reason we need this change now?
There was a problem hiding this comment.
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.
why does this change happen?
There was a problem hiding this comment.
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.
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.
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.
We are planning on reverting this right?
There was a problem hiding this comment.
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.
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.
nit. this change should be in a different pr right?
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