Skip to content

chore: Consolidate wheels/source builds & artifacts for release - #5119

Merged
franciscojavierarceo merged 1 commit into
feast-dev:masterfrom
tchughesiv:sdist
Mar 5, 2025
Merged

chore: Consolidate wheels/source builds & artifacts for release#5119
franciscojavierarceo merged 1 commit into
feast-dev:masterfrom
tchughesiv:sdist

Conversation

@tchughesiv

@tchughesiv tchughesiv commented Mar 5, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Since the 0.42.0 release, our workflow has no longer been uploading the sdist tarball to PyPi. This PR aims to fix this by consolidating the wheels and source builds to a single job and uploading said artifacts to the same temp storage which feeds the PyPi upload job.

It's worth noting that the separate source job appears to have previously been created to resolve some issues that the make install-go-ci-dependencies was causing. This make command is no longer in use as it has since been commented out.

feast/Makefile

Lines 649 to 651 in 75f5a90

#install-go-ci-dependencies:
# go install golang.org/x/tools/cmd/goimports
# python -m pip install "pybindgen==0.22.1" "grpcio-tools>=1.56.2,<2" "mypy-protobuf>=3.1"

@tchughesiv

Copy link
Copy Markdown
Contributor Author

@franciscojavierarceo in troubleshooting this fix... i'm realizing the build-docker-images pre-release job will never pass since it depends on a published feast version in PyPi.

@tchughesiv

Copy link
Copy Markdown
Contributor Author

@franciscojavierarceo scratch that ... i see we're building the dev images for pre-release. all good.

@franciscojavierarceo

Copy link
Copy Markdown
Member

@franciscojavierarceo in troubleshooting this fix... i'm realizing the build-docker-images pre-release job will never pass since it depends on a published feast version in PyPi.

Yeah, that I had made sure of.

Comment thread .github/workflows/build_wheels.yml
@tchughesiv
tchughesiv marked this pull request as draft March 5, 2025 16:01
@tchughesiv
tchughesiv marked this pull request as ready for review March 5, 2025 16:05
@tchughesiv
tchughesiv marked this pull request as draft March 5, 2025 16:12
@tchughesiv
tchughesiv force-pushed the sdist branch 2 times, most recently from 31d290b to a815a47 Compare March 5, 2025 16:19
@tchughesiv
tchughesiv marked this pull request as ready for review March 5, 2025 16:19
Signed-off-by: Tommy Hughes <tohughes@redhat.com>
@tchughesiv tchughesiv changed the title fix: Consolidate wheels/source builds & artifacts for release chore: Consolidate wheels/source builds & artifacts for release Mar 5, 2025
@tchughesiv

tchughesiv commented Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

@tokoko do you know why we were using a macos runner to separately handle our sdist build?

with:
python-version: "3.11"
architecture: x64
- name: Setup Node

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we don't need to setup node?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, but we're consolidating and it's already being set in the new job

@tchughesiv tchughesiv Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

check line # 41

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh duh, nice thank you

@tokoko

tokoko commented Mar 5, 2025

Copy link
Copy Markdown
Collaborator

@tchughesiv nope, it was macos from the get-go, shouldn't make a difference one way or the other. I suspect the point of a separate job was simply to run it in parallel.

@franciscojavierarceo
franciscojavierarceo merged commit 2ac4906 into feast-dev:master Mar 5, 2025
@franciscojavierarceo

Copy link
Copy Markdown
Member

@tchughesiv the local integration tests ran for like 5+ hours so I just merged it...that sounds like something we should look into.

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.

4 participants