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

Fixed Windows CircleCI errors, and add tests for utils #313

Merged
merged 11 commits into from Jan 28, 2020

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Jan 21, 2020

After some testing in the Windows CircleCI instance, it turns out that there's some issues in installing XGBoost in Windows environments. As noted in the PyPi page, pip installation doesn't always work in Windows.

This commit installs XGBoost beforehand using Conda as a workaround for the pip install previously used.

@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

Merging #313 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #313   +/-   ##
=======================================
  Coverage   97.06%   97.06%           
=======================================
  Files          96       96           
  Lines        2963     2963           
=======================================
  Hits         2876     2876           
  Misses         87       87

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dda9552...4323d40. Read the comment docs.

@christopherbunn christopherbunn requested a review from dsherry Jan 21, 2020
command: |
C:\Users\circleci\Miniconda3\shell\condabin\conda-hook.ps1
conda activate curr_py
conda install -c anaconda py-xgboost
Copy link
Collaborator

@dsherry dsherry Jan 22, 2020

Choose a reason for hiding this comment

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

Is this run command invoked somewhere during circleci setup? I don't see this PR adding a call to it anywhere. (I'm guessing that's because I don't understand yet how circleci works with this config :) )

Copy link
Contributor Author

@christopherbunn christopherbunn Jan 24, 2020

Choose a reason for hiding this comment

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

When CircleCI runs a workflow it automatically runs all of the steps within a job. So this run command is invoked as part of the win_unit_tests job

Copy link
Collaborator

@dsherry dsherry Jan 27, 2020

Choose a reason for hiding this comment

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

Awesome

@kmax12
Copy link
Contributor

kmax12 commented Jan 23, 2020

we probably want to add something about this to an install page in the docs for any windows users of evalml

this is also a good reason to have minimal required dependencies in requirement.txt since some users may have difficult installing some third part libraries.

related to discussion in #315

@christopherbunn christopherbunn requested a review from dsherry Jan 27, 2020
@dsherry dsherry changed the title Fixed Windows CircleCI errors Fixed Windows CircleCI errors, and add tests for utils Jan 27, 2020
Copy link
Collaborator

@dsherry dsherry left a comment

@christopherbunn I agree with Max's comment: can you please add a short note to the doc about the Windows pip xgboost issue? My suggestion: in the documentation front page, at the end of the Install section (after the pip command), add something like: "A note for Windows users: the xgboost library may not be pip-installable in some environments. If so, please install it from Github instead."
(with the links).

After that this PR is ready to go.

Also, a note: technically this PR includes two separate code changes: a) fixing Windows CircleCI errors, and b) testing for utils. Please put separate issues in separate PRs in the future :) it makes it easy to understand the history on master.

Thanks!

@christopherbunn
Copy link
Contributor Author

christopherbunn commented Jan 28, 2020

@dsherry Added the requested line at the end of the install section.

Now that you brought it up, I am now noticing that there are testing changes for the utils. I didn't write these changes, in fact they are exactly the same as the current changes in master. This branch is the first time I've tried to rebase the branch, could it be that I did that process incorrectly?

Copy link
Collaborator

@dsherry dsherry left a comment

Just worked on rebasing branch off master with Chris. LGTM now 👍

@christopherbunn christopherbunn merged commit 9a583a6 into master Jan 28, 2020
2 checks passed
@christopherbunn christopherbunn deleted the sklearn-win-circleci branch Jan 28, 2020
@angela97lin angela97lin mentioned this pull request Mar 9, 2020
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.

None yet

3 participants