Skip to content

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

Merged
merged 9 commits into from
Aug 23, 2022

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Aug 18, 2022

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 the py38_conda_defaults_openblas uses using SKLEARN_TESTS_GLOBAL_RANDOM_SEED=1 in a PR).

You could do a combination of all random seeds and float32 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!`

Copy link
Member

@thomasjpfan thomasjpfan 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 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.

@lesteve
Copy link
Member Author

lesteve commented Aug 19, 2022

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 get_commit_message.py to be able to only print the commit message and be used directly in a bash script.

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 my-script.sh the dependency output would be available as $MYVARFROMJOBA.

@lesteve
Copy link
Member Author

lesteve commented Aug 19, 2022

Side-comment by printing the environment, I found out SYSTEM_PULLREQUEST_SOURCECOMMITID could be used to get the commit of the pull request rather than relying on the merge commit message having the commit id as the second word. Quite a minor improvement though ...

SYSTEM_PULLREQUEST_SOURCECOMMITID does not seem documented but it is officially supported see this

@lesteve
Copy link
Member Author

lesteve commented Aug 19, 2022

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 [float32] marker, this way the CI will be green.

Copy link
Member

@thomasjpfan thomasjpfan 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 for the update! LGTM

Copy link
Member

@ogrisel ogrisel left a 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.

@ogrisel ogrisel merged commit 68c5ceb into scikit-learn:main Aug 23, 2022
@lesteve lesteve deleted the float32-commit branch August 23, 2022 09:06
@lesteve
Copy link
Member Author

lesteve commented Aug 23, 2022

Side-comment: the posix-docker.yml changes meant for #24201 were actually merged in this PR (probably my fault).

This may break the nightly build until #24198 is merged.

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.

3 participants