-
-
Notifications
You must be signed in to change notification settings - Fork 26k
CI add ability to run float32 tests by commit message #24202
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! Overall, I think this is a good idea. As for the commit tag, [float32]
is okay for me even tho it is not completely consistent with the other tags.
Indeed there was some temporary debug messages and I was hoping that there was a variable we could use from Azure DevOps but apparently not ... The simplest thing I found was to modify I did not find anything straightforward in the Azure DevOps doc to be able to use a dependency output variable inside a bash script. Let me know if you know of anything. The only thing I found would be to set a variable in the yaml file from a dependency output like this: jobs:
- job: A
steps:
- bash: |
echo "##vso[task.setvariable variable=myOutputVar;isoutput=true]this is from job A"
name: passOutput
- job: B
dependsOn: A
variables:
myVarFromJobA: $[ dependencies.A.outputs['passOutput.myOutputVar'] ]
steps:
- bash: ./my-script.sh
My understanding is that inside |
Side-comment by printing the environment, I found out
|
OK this seems to be working as expected in my last commit e9e9085. The debian 32bit failure is known and will be tackled in a separate PR. I'll push a commit with some clean-up and without the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update! LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well. Thanks for the fix and sorry for the slow review. Maybe put the "Quick Review" label next time if we you need a second review.
I think CI PRs like this are fine to merge with a single review as they are not likely to impact our users negatively.
In #24198 I realised there were no way to run float32 tests in all builds in a PR. That means that PRs using the
global_dtype
fixture are mostly tested in the nighly scheduled build (currently only thepy38_conda_defaults_openblas
uses usingSKLEARN_TESTS_GLOBAL_RANDOM_SEED=1
in a PR).You could do a combination of
all random seeds
andfloat32 test
to have better coverage of likely problems before merging the PR rather than picking issues in a later scheduled build (or relying on running the tests locally).About the name: I tried to follow the other patterns for the other commit markers e.g.
ci skip
,lint skip
where the verb is at the end. Using[float32]
could work but at the same time, it is not a separate build in contrary to the other one-word ones ([pypy]
,[scipy-dev]
, etc ...)More than happy to change the name if better suggestions come along!`