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

Fix install test and uncap kaleido #4423

Merged
merged 31 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3505b67
install test fix
MichaelFu512 May 16, 2024
ddcc0f2
release notes
MichaelFu512 May 16, 2024
381bd2d
kaleido up
MichaelFu512 May 16, 2024
9975d03
cap vowpalwabbit
MichaelFu512 May 17, 2024
1ee459d
forgot quotes
MichaelFu512 May 17, 2024
ce422b5
add homebrew to try and fix lightgbm
MichaelFu512 May 17, 2024
b00d604
remove things related to vowpalwabbit
MichaelFu512 May 17, 2024
867090c
release notes and __init__
MichaelFu512 May 17, 2024
ac13851
latest depdendency
MichaelFu512 May 17, 2024
cf097ee
fix test
MichaelFu512 May 20, 2024
8f3af8c
idk codecov
MichaelFu512 May 20, 2024
8b6288f
codecov 2
MichaelFu512 May 20, 2024
314460c
Merge branch 'main' into remove_vowpalwabbit
MichaelFu512 May 20, 2024
d51cac3
Merge branch 'main' into remove_vowpalwabbit
MichaelFu512 May 20, 2024
54d3753
release notes
MichaelFu512 May 21, 2024
9e00b9e
Merge branch 'main' into install_test_fix
MichaelFu512 May 21, 2024
6c23f1c
Merge branch 'remove_vowpalwabbit' into install_test_fix
MichaelFu512 May 21, 2024
787a2a5
uncapp pytest-related things
MichaelFu512 May 21, 2024
4213a84
attempting kaleido fix
MichaelFu512 May 21, 2024
1c7276d
don't use cache
MichaelFu512 May 21, 2024
5f8a18a
remove if statement
MichaelFu512 May 21, 2024
bceff5b
Merge branch 'main' into install_test_fix
MichaelFu512 May 21, 2024
f5b99ca
test 3
MichaelFu512 May 21, 2024
34996d5
Revert "test 3"
MichaelFu512 May 21, 2024
a002301
force kaleido to be headless
MichaelFu512 May 21, 2024
ebb35cd
unpinned and bumped kaleido
MichaelFu512 May 21, 2024
dd39558
release note update
MichaelFu512 May 21, 2024
1d9201b
release note but actually
MichaelFu512 May 21, 2024
c65fa71
docstring update
MichaelFu512 May 21, 2024
cd3f4f8
removed pytest changes (do it in another mr)
MichaelFu512 May 21, 2024
2b11a69
becca's comments
MichaelFu512 May 22, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/meta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ outputs:
run:
- '{{ pin_subpackage("evalml-core", max_pin="x.x.x.x") }}'
- plotly >=5.0.0
- python-kaleido ==0.1.0
- python-kaleido >=0.2.0
- matplotlib-base >=3.3.3
- seaborn >=0.11.1
- ipywidgets >=7.5
Expand Down
13 changes: 5 additions & 8 deletions .github/workflows/install_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ jobs:
matrix:
os: [ubuntu-latest, macos-latest]
python_version: ["3.9", "3.10", "3.11"]
exclude:
- os: macos-latest
python-version: "3.8"
runs-on: ${{ matrix.os }}
steps:
- name: Checkout repository
Expand All @@ -37,16 +34,16 @@ jobs:
with:
path: ${{ env.pythonLocation }}
key: ${{ matrix.os- }}-${{ matrix.python_version }}-install-${{ env.pythonLocation }}-${{ hashFiles('**/pyproject.toml') }}-v01
- name: Set up Homebrew
id: set-up-homebrew
uses: Homebrew/actions/setup-homebrew@master
- name: Set up cmake and libomp
run: brew install cmake libomp
eccabay marked this conversation as resolved.
Show resolved Hide resolved
- name: Build evalml package
run: make package
- name: Install evalml from sdist (not using cache)
if: steps.cache.outputs.cache-hit != 'true'
MichaelFu512 marked this conversation as resolved.
Show resolved Hide resolved
run: |
python -m pip install "unpacked_sdist/."
- name: Install evalml from sdist (using cache)
if: steps.cache.outputs.cache-hit == 'true'
run: |
python -m pip install "unpacked_sdist/." --no-deps
Copy link
Contributor Author

@MichaelFu512 MichaelFu512 May 21, 2024

Choose a reason for hiding this comment

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

Whenever the install_tests would use cache, the test would fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, do you think it's noticably slower since we're not using the cache? If so, by how much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly the time it takes varies.

The latest commit (2b11a69) has the time that ubuntu takes for this step to be 8 seconds. For MacOS it takes 40 seconds for python3.9 and 3 minutes for python3.11.

Conversely, the second latest commit (cd3f4f8) has the time that ubuntu takes hover around 1 minute. MacOS hovers at around 40 seconds.

I can't say for sure how much slower it is without cache because it didn't work with cache enabled. I can go test to see if I can get cache working (hopefully it doesn't erase the approvals).

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I was having trouble with caches in other OS repos recently and ended up removing them because they were failing too often.

- name: Test by importing packages
run: |
python -c "import evalml"
Expand Down
2 changes: 2 additions & 0 deletions docs/source/release_notes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ Release Notes
* Dropped support for Python 3.8 :pr:`4414`
* Removed vowpalwabbit :pr:`4427`
* Uncapped holidays :pr:`4428`
* Unpinned kaleido :pr:`4423`
* Documentation Changes
* Testing Changes
* Run airflow tests in Python 3.9 :pr:`4391`
* Remove iterative test from airflow runs :pr:`4424`
* Update GH actions to improve handling of potentially unsafe variables :pr:`4417`
* Fix install test :pr:`4423`

.. warning::

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ graphviz==0.20.3
holidays==0.49
imbalanced-learn==0.12.2
ipywidgets==8.1.2
kaleido==0.1.0
kaleido==0.2.1
lightgbm==4.3.0
lime==0.2.0.1
matplotlib==3.9.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ graphviz==0.13
holidays==0.13
imbalanced-learn==0.11.0
ipywidgets==7.5
kaleido==0.1.0
kaleido==0.2.0
lightgbm==4.0.0
lime==0.2.0.1
matplotlib==3.3.3
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ graphviz==0.13
holidays==0.13
imbalanced-learn==0.11.0
ipywidgets==7.5
kaleido==0.1.0
kaleido==0.2.0
lightgbm==4.0.0
lime==0.2.0.1
matplotlib==3.3.3
Expand Down
13 changes: 8 additions & 5 deletions evalml/tests/utils_tests/test_gen_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,16 @@
def in_container_arm64():
"""Helper fixture to run chromium as a single process for kaleido.

The env var is set in the Dockerfile.arm for the purposes of local
testing in a container on a mac M1, otherwise it's a noop.
Useful as kaleido > 0.1.0 on windows seems to plotly.write_image to hang indefinitely.
Makes tests that use plotly not hang and thus pass.
MichaelFu512 marked this conversation as resolved.
Show resolved Hide resolved
"""
if os.getenv("DOCKER_ARM", None):
import plotly.io as pio
import plotly.io as pio

Check warning on line 40 in evalml/tests/utils_tests/test_gen_utils.py

View check run for this annotation

Codecov / codecov/patch

evalml/tests/utils_tests/test_gen_utils.py#L40

Added line #L40 was not covered by tests

pio.kaleido.scope.chromium_args += ("--single-process",)
pio.kaleido.scope.chromium_args += (

Check warning on line 42 in evalml/tests/utils_tests/test_gen_utils.py

View check run for this annotation

Codecov / codecov/patch

evalml/tests/utils_tests/test_gen_utils.py#L42

Added line #L42 was not covered by tests
"--single-process",
"--headless",
"--disable-gpu",
)
Copy link
Contributor Author

@MichaelFu512 MichaelFu512 May 21, 2024

Choose a reason for hiding this comment

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

Originally these were only meant for mac m1 tests to pass, but windows tests also needs these settings for testing, else some tests hang forever, causing workers to fail.



@patch("importlib.import_module")
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ dependencies = [
"nlp-primitives >= 2.9.0",
"networkx >= 2.7",
"plotly >= 5.0.0",
"kaleido == 0.1.0",
"kaleido >= 0.2.0",
"ipywidgets >= 7.5",
"xgboost >= 1.7.0.post0",
"catboost >= 1.1.1",
Expand Down
Loading