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

Add dev package support to build_packages.yml. #667

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

ScottTodd
Copy link
Member

@ScottTodd ScottTodd commented Dec 9, 2024

Progress on #584. Depends on #666 (the first commit).

This is refactors the build_packages.yml workflow so it can be used via workflow_call as part of a "pkgci" setup, as an alternative to creating a new pkgci_build_packages.yml workflow as originally proposed in #589. This lets us reuse the same workflow for building stable, nightly, and dev packages, all across the same matrix of Python versions and operating systems. Package builds take about 2 minutes (wall time) across the full matrix, so we might as well build them all, instead of artificially constraining ourselves to a subset like only Linux on Python 3.11.

Triggers for the workflow are now this:

Trigger Scenario Build type(s)
schedule Nightly pre-release build rc
workflow_dispatch Workflow testing, manual releasing rc default, stable and dev possible
workflow_call Pull request or push "pkgci" dev builds dev default, stable and rc possible

With this workflow behavior:

Build type Version suffix Cache enabled? Tracing enabled? Pushes to release?
stable None No Yes No
rc rcYYYYMMDD No Yes Yes
dev .dev0+${{ github.sha }} Yes No No

Tested over at https://github.com/ScottTodd/shark-ai/actions/workflows/build_packages.yml. Example run: https://github.com/ScottTodd/shark-ai/actions/runs/12245900071 (warm cache)

@ScottTodd ScottTodd requested a review from marbre December 9, 2024 23:21
Comment on lines 46 to 64
# Compute version suffix based on inputs (default to 'rc')
- name: Compute stable version suffix
if: ${{ inputs.build_type == 'stable' }}
run: |
version_suffix=""
echo "version_suffix=${version_suffix}" >> $GITHUB_ENV
- name: Compute rc version suffix
if: ${{ inputs.build_type == 'rc' || inputs.build_type == '' }}
run: |
version_suffix="$(printf 'rc%(%Y%m%d)T')"
echo "version_suffix=${version_suffix}" >> $GITHUB_ENV
- name: Compute dev version suffix
if: ${{ inputs.build_type == 'dev' }}
run: |
version_suffix=".dev0+${{ github.sha }}"
echo "version_suffix=${version_suffix}" >> $GITHUB_ENV
Copy link
Member Author

@ScottTodd ScottTodd Dec 9, 2024

Choose a reason for hiding this comment

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

We could also move this logic into compute_local_version.py, as it is in IREE: https://github.com/iree-org/iree/blob/ab3c9bb031f82f8481da7d867fab03db9180646a/build_tools/python_deploy/compute_local_version.py#L26-L30

We'd then need to pass the selection from the workflow inputs through to the script somehow:

      - name: Generate release candidate versions
        id: version_rc
        run: |
          python3 build_tools/python_deploy/compute_local_version.py --release-type ${{ inputs.build_type }} sharktank
          # extract version_suffix from the local version to forward to the job output?
          # that is currently passed to `write_requirements.py` down below as part of building the shark-ai package
release_type = parser.add_mutually_exclusive_group()
release_type.add_argument("-stable", "--stable-release", action="store_true")  # default
release_type.add_argument("-rc", "--nightly-release", action="store_true")
release_type.add_argument("-dev", "--development-release", action="store_true")
release_type.add_argument("--custom-string", action="store", type=str)
release_type.add_argument("--build-type", action="store", type=str)

args = parser.parse_args()
if args.build_type == "dev":
  args.development_release = True
elif args.build_type == "stable":
  ...

I generally prefer to keep workflow files simpler and put logic in scripts that can be run outside of the CI. Feeling lazy about this though, since it is already working as-is :P. Opinions?

Copy link
Collaborator

@marbre marbre Dec 10, 2024

Choose a reason for hiding this comment

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

Well I would like to harmonize what we have here and what I've done for IREE in general. However, I am fine with going what you have right now (assuming that one of us fills an issue, see my review comment about refactoring to reduce confusion).

TL;DR: Being lazy is fine here and it would probably good to refactor here and in IREE.

Comment on lines +10 to +16
# Trigger from another workflow (typically to build dev packages and then test them)
workflow_call:
inputs:
build_type:
description: The type of build version to produce ("stable", "rc", or "dev")
type: string
default: "dev"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is refactors the build_packages.yml workflow so it can be used via workflow_call as part of a "pkgci" setup, as an alternative to creating a new pkgci_build_packages.yml workflow as originally proposed in #589. This lets us reuse the same workflow for building stable, nightly, and dev packages, all across the same matrix of Python versions and operating systems. Package builds take about 2 minutes (wall time) across the full matrix, so we might as well build them all, instead of artificially constraining ourselves to a subset like only Linux on Python 3.11.

Sample workflow run with that setup:

https://github.com/ScottTodd/shark-ai/actions/runs/12246364049

image

.github/workflows/build_packages.yml Outdated Show resolved Hide resolved
.github/workflows/build_packages.yml Outdated Show resolved Hide resolved
echo "version_suffix=${version_suffix}" >> $GITHUB_OUTPUT
python3 build_tools/python_deploy/compute_local_version.py --version-suffix=${version_suffix} sharktank
python3 build_tools/python_deploy/compute_local_version.py --version-suffix=${version_suffix} shortfin
python3 build_tools/python_deploy/compute_common_version.py -rc --version-suffix=${version_suffix} --write-json
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this works (as the version suffix is empty for stable releases and overwrites for dev releases) but we should refactor in a follow up (happy to take this) as it rather confusing that -rc is passed for all builds. I needed to look twice here to understand why it is probably fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I missed the -rc in the middle there. Yeah, it still works but we should refactor :P

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is addressed with #674.

@ScottTodd
Copy link
Member Author

Rebased + applied suggestions.

@ScottTodd ScottTodd enabled auto-merge (squash) December 10, 2024 16:07
@ScottTodd ScottTodd merged commit b585963 into nod-ai:main Dec 10, 2024
8 checks passed
@ScottTodd ScottTodd deleted the package-inputs branch December 10, 2024 16:08
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