fix failing e2e test on windows #25
Conversation
.github/actions/e2e/action.yml
Outdated
| @@ -33,5 +33,5 @@ runs: | |||
|
|
|||
| - name: Run tests | |||
| working-directory: ${{ inputs.working_directory }} | |||
| run: pytest -v | |||
| run: pytest -v -p no:faulthandler | |||
There was a problem hiding this comment.
Ref for the fix: https://stackoverflow.com/questions/57523762/pytest-windows-fatal-exception-code-0x8001010d
Thanks @AleksanderWWW for finding it.
There was a problem hiding this comment.
Do I understand it correctly: we try to render some plot and Windows runner doesn't support GUI in general so there is some exception that was ignored in pytest<5.0.0?
How about letting matplotlib to know that there is no GUI available? I found this: https://stackoverflow.com/questions/4931376/generating-matplotlib-graphs-without-a-running-x-server
And this looks like a config that could be common for all of our integrations. How about moving it to pyproject.toml?
There was a problem hiding this comment.
- Do you mean adding this
matplotlib.use('Agg') - Sounds good, but is it necessary to add it to all of our integrations?
There was a problem hiding this comment.
Ad 1. Yes, we can try another approach with this line added in e2e tests (before most of imports). But this no-faulthandler is fine.
Ad 2. No, just try to move it to pyproject.toml.
There was a problem hiding this comment.
pytest: error: unrecognized arguments: --no-faulthandler
seems that ChatGPT deceived us 😄
There was a problem hiding this comment.
Ohh, so go ahead with just a -p no:faulthandler flag 😉
src/neptune_xgboost/impl/__init__.py
Outdated
| @@ -104,7 +107,7 @@ def __init__( | |||
| log_tree=None, | |||
| tree_figsize=30, | |||
| ): | |||
|
|
|||
| use("Agg") | |||
There was a problem hiding this comment.
We shouldn't set this from our end as any call to matplotlib after calling our callback will use Agg backend.
I think previous fix was better as it only affected tests not user code.
There was a problem hiding this comment.
Of course, this should be at most part of e2e tests, not the code itself 😉
There was a problem hiding this comment.
My bad, I meant to add it to e2e tests, not callback implementation
|
@Raalsky @kshitij12345 |
Does the second option with matplotlib works without a faulthandler flag? |
Yes :) |
No description provided.