Skip to content

[MRG] Adds 32bit linux testing to CI#14225

Merged
rth merged 19 commits intoscikit-learn:masterfrom
thomasjpfan:32bit-linux
Jul 2, 2019
Merged

[MRG] Adds 32bit linux testing to CI#14225
rth merged 19 commits intoscikit-learn:masterfrom
thomasjpfan:32bit-linux

Conversation

@thomasjpfan
Copy link
Copy Markdown
Member

Reference Issues/PRs

Closes #9352

What does this implement/fix? Explain your changes.

Uses docker to run tests on 32bit linux.

Any other comments?

Besides the known 32bit failure: test_extract_xi. This PR finds another error in test_gradient_and_hessian_sanity.

-e OPENBLAS_NUM_THREADS=$OPENBLAS_NUM_THREADS
-e SKLEARN_SKIP_NETWORK_TESTS=$SKLEARN_SKIP_NETWORK_TESTS
i386/ubuntu:16.04
sleep 1000000
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.

I've not seen this trick before!

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.

Actually I do not understand: why are the subsequent steps being executed before the completion of this step?

Edit:

Oh I seed, you used the -d flag to detach the container and let it run in the background. Maybe you could explain this with a comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a comment about using detached along with using -detach to be more explicit.

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.

If the installed version of sleep supports units other than seconds, please specify in minutes.

displayName: 'Test Library'
- task: PublishTestResults@2
inputs:
testResultsFiles: '$(TEST_DIR)/$(JUNITXML)'
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.

I'm confused about how this accesses the docker container

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The $TEST_DIR is mapped into to the containers /temp_dir, so the junit xml generated by pytest will be place correctly into $TEST_DIR for the host to see.

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.

Ah, I missed the -v

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Jul 1, 2019

The test_gradient_and_hessian_sanity shows very large values in the gradients array, as if it wasn't properly initialized or if some overflow had happened.

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Jul 1, 2019

Should this pr skip test_gradient_and_hessian_sanity in 32 bit so it can be merged?

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Very nice! I agree we should probably mark the failing test a know failure on 32 bit arch and open an issue about it.

@thomasjpfan
Copy link
Copy Markdown
Member Author

PR updated with skipping test_gradient_and_hessian_sanity and opening an issue to discuss it: #14236

@rth
Copy link
Copy Markdown
Member

rth commented Jul 2, 2019

Great, thanks @thomasjpfan !

@rth rth merged commit 29b9bb8 into scikit-learn:master Jul 2, 2019
@thomasjpfan
Copy link
Copy Markdown
Member Author

@rth Next time I'll be more specific in saying this is for Azure. 😅

@rth
Copy link
Copy Markdown
Member

rth commented Jul 2, 2019

BLD/CI Adds 32bit linux testing to Appveyor

Aww yes, sorry it was a long day :)

(also they do both start by an A)

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
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.

Add travis configuration to run the tests on 32 bit linux

4 participants