Skip to content

debug: errorlevel conditionals #2285

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

Merged
merged 4 commits into from
Jan 28, 2025
Merged

Conversation

jharlow-intel
Copy link
Contributor

Wheels tests were failing because some python abrupt failures (or windows access violations) would use an exit code that was both non-zero and non-1.

This should capture everything that isn't an exit code of 0

antonwolfy
antonwolfy previously approved these changes Jan 27, 2025
Copy link
Contributor

@antonwolfy antonwolfy left a comment

Choose a reason for hiding this comment

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

Thank you @jharlow-intel for resolving that 👍

@coveralls
Copy link
Collaborator

coveralls commented Jan 27, 2025

Coverage Status

coverage: 71.316%. remained the same
when pulling 5422dc9 on harlowjo/debug/wheels-tests
into c844b26 on master.

Copy link
Contributor

View rendered docs @ https://intelpython.github.io/dpnp/pull/2285/index.html

Copy link
Contributor

github-actions bot commented Jan 27, 2025

Array API standard conformance tests for dpnp=0.17.0dev5=py312he4f9c94_13 ran successfully.
Passed: 966
Failed: 1
Skipped: 33

@jharlow-intel
Copy link
Contributor Author

jharlow-intel commented Jan 28, 2025

Hmm, I was a little afraid of that. I guess the tests can all "pass" - but due to how some tests are muted I guess it doesn't always return an exit code of 0 even when it passes.

I'll need to experiment more, and I'll share my findings with you in Teams.

We at least now know it is indeed the run_test.bat script that was returning a 0 code. We just need to adjust that or the pytest command within maybe.

edit:
Noticed some error logging related to the neq conditionals, I believe it might have been a syntax error. Trying the fix now

@jharlow-intel
Copy link
Contributor Author

https://cje-fm-owrp-prod04.devtools.intel.com/satg-dap-intelpython/job/intel-packages/job/dpnp/job/dev-windows-py3.12/job/test/1194/console

Looks like it's passing and failing when appropriate now 😄

Copy link
Contributor

@antonwolfy antonwolfy left a comment

Choose a reason for hiding this comment

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

LGTM! Works as expected now. Thank you @jharlow-intel

@jharlow-intel
Copy link
Contributor Author

It looks like my conditional is basically identical to the ones used previously, but their is a nuance to errorlevel vs %errorlevel%`

TLDR; these variables are technically not the same and errorlevel is sytem-level and %errorlevel% will default to the system-level value, unless it's set manually by something else (for example, pytest or python itself could be setting %errorlevel%)

Links I read:
https://stackoverflow.com/questions/42967840/errorlevel-vs-errorlevel-vs-exclamation-mark-errorlevel-exclamation-mark
https://devblogs.microsoft.com/oldnewthing/20080926-00/?p=20743

I also peeked at conda-forge recipes and they also seem to lean into the %ERRORLEVEL% neq 0 syntax so I think this is for the best here

@antonwolfy antonwolfy merged commit 91161a8 into master Jan 28, 2025
62 of 70 checks passed
@antonwolfy antonwolfy deleted the harlowjo/debug/wheels-tests branch January 28, 2025 18:23
github-actions bot added a commit that referenced this pull request Jan 28, 2025
Wheels tests were failing because some python abrupt failures (or
windows access violations) would use an exit code that was both non-zero
and non-1.

This should capture everything that isn't an exit code of 0. 91161a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants