-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
MNT Fixes failing test_keras #794
Conversation
Thanks for the PR @thomasjpfan. Looks like the tests are CI tests are failing because of an unrelated linting issue w/ mypy. linux38 CI job: linting output
|
It seems like this one is good to merge then? |
This PR LGTM. |
Do you have merge access @stsievert? I don't :) |
Nope, no merge access. I know @TomAugspurger does though. |
And probably @jrbourbeau as well :) Seems like this one is good to go |
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.
Thanks @thomasjpfan! Merging as this is a definite improvement over our current CI failure situation. However I did want to point out that it appears this test isn't run on CI and is instead skipped here
pytestmark = pytest.mark.skip(reason="Missing tensorflow or scikeras") |
In the future it'd be good to make sure we have a CI build with tensorflow and scikeras installed
Good point @jrbourbeau. It looks like keras and tensorflow are only installed when the branch is main. So the test ran once it was merged. I am not sure if that is intentional or not. @thomasjpfan when it ran there it passed on all builds except earliest: https://dev.azure.com/dask-dev/6d1a2a76-7181-452f-943c-c034bbab2bfa/_apis/build/builds/1961/logs/39 this might just be an instance of we should bump the minimum requirements. |
Hm... SciKeras raises an error if |
Fixes failing test on the main branch