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

[ci] fix ci to test windows properly #3543

Merged
merged 1 commit into from
Nov 26, 2024
Merged

[ci] fix ci to test windows properly #3543

merged 1 commit into from
Nov 26, 2024

Conversation

siddvenk
Copy link
Contributor

@siddvenk siddvenk commented Nov 25, 2024

Description

The continuous workflow for windows is busted, and we've had a silent failure in that test for a long time.

The issue is that we run 3 separate tests in the final github step of continuous/build-windows, but the success/failure outcome is only dependent on the final command we run. Specifically, we run:

./gradlew -Pjni "-Dai.djl.default_engine=PyTorch" test -x examples:test
./gradlew -Pjni --rerun-tasks "-Dai.djl.default_engine=TensorFlow" test -x examples:test
./gradlew --rerun-tasks "-Dai.djl.default_engine=OnnxRuntime" :integration:test

When this runs if the 3rd line succeeds, it hides any failure that occurs in the previous two lines.

This change refactors the continuous workflow by unifying the windows test into the existing mac/linux tests, which function as expected. The separate stage for windows may have made sense sometime in the past, it no longer needs to be separate.

I have also modified the nightly-publish workflow to ensure that we test all 3 major OS's across each of the suites. I have also specified fail-fast: false to ensure no short-circuiting of tests due to failures.

@siddvenk siddvenk requested review from zachgk and a team as code owners November 25, 2024 23:03
@siddvenk
Copy link
Contributor Author

Note:

The continuous/build (windows-latest) is expected to fail as a result of this change. We are fixing the workflow so that failures like this are exposed. I will be raising another PR shortly that will address that issue. I want to merge this first so that we can see that the change I make actually resolves the issue.

Same case for the nightly-publish. I have the run based on the changes made here available at https://github.com/deepjavalibrary/djl/actions/runs/12020282449

@siddvenk siddvenk merged commit 73f7cb0 into master Nov 26, 2024
22 of 25 checks passed
@siddvenk siddvenk deleted the windows-ci branch November 26, 2024 00:56
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