MAINT: Update for latest VTK #1050
Conversation
|
It would also be good to add a CI job that tests with |
|
@larsoner It seems to fix the hang I was seeing, thanks. |
|
On Fedora 34 with Mayavi-4.7.3 including this fix here, vtk-9.0.1 and python 3.9.5, Mayavi2 (the app) now starts and plots but throws some error (see below). |
|
VTK just released 9.0.2, and all my |
|
@mjg for your error I would open a bug with the VTK folks (of course after some searching), it's possible they will recognize something from the traceback: https://gitlab.kitware.com/vtk/vtk/-/issues Alternatively you could engage in a time consuming |
|
What's the latest on this please? I can't install mayavi, it just seems to hit a wall and get stuck! |
There was a problem hiding this comment.
Thank @larsoner! This looks good to me, I will refactor the print debug statements in a separate PR.
|
@larsoner -- I apologize for the delay in getting this done. I am going to try to get a bit more regular on this going forward. |
|
Even on this branch on 9.1.0-rc2 wheels on macOS at least I get: and then after fixing that: I can push a commit to work around (fix?) those here or in a separate PR @prabhuramachandran , let me know which is better |
|
... and then after fixing those: |
|
Clearly my workarounds are not good, or something else is wrong :( |
|
And this exists: So I'm not sure where/why the traits-based wrapper is failing :( |
|
@larsoner -- I will merge this for now to make life easier for everyone, I will look at the additional failures for 9.1.0 tomorrow and will not be able to look at this today. Looking at the errors, it looks like 9.1.0 changed the docstring signature which is going to break many things for sure. Thanks for bringing this up. My first goal is to get 9.0.3 all set and the tests going there and then I will look at 9.1.0-rc2. Am hoping that by next weekend things will be in better shape. |
|
@rahulporuri -- I am not able to merge this since the tests are required and don't have the time to sort this out today, so if you are able to merge this, please do so so I can build on top of this to fix additional issues. Thanks. |
Codecov Report
@@ Coverage Diff @@
## master #1050 +/- ##
==========================================
- Coverage 49.83% 49.83% -0.01%
==========================================
Files 263 263
Lines 23885 23887 +2
Branches 3248 3249 +1
==========================================
Hits 11903 11903
- Misses 11210 11211 +1
- Partials 772 773 +1
Continue to review full report at Codecov.
|
|
I've merged the master branch into this one; that should sort out the test requirements. |
| @@ -640,7 +643,10 @@ def _find_get_set_methods(self, klass, methods): | |||
|
|
|||
| # Find the default and range of the values. | |||
| if gsm: | |||
| # Useful for debugging on failures: | |||
There was a problem hiding this comment.
For future evolution of this code, how about using logging in place of the commented-out print statements?
There was a problem hiding this comment.
This code is primarily used at build time, so would logging be appropriate? I was thinking of using an environment variable like "MAYAVI_DEBUG_BUILD" or so to turn on these print statements. What do you recommend?
There was a problem hiding this comment.
Not sure, but just from a code cleanliness point of view I'd really like to get away from having commented-out code in the codebase.
A "verbose" or "debug" flag to turn on the print statements doesn't seem unreasonable; that's probably better than using an environment variable at this point - we probably want the environment variable handling in the topmost layer.
|
@mdickinson -- who am I to talk to about the admin privileges? Until this point I was merging some of the PRs but I am not sure if it is something on my end or in the setup of the repo. |
|
@prabhuramachandran The inability to merge was purely related to the PR, not to you. I wasn't being allowed to merge either. :-) Everyone in the "enthought" team in the "enthought" org currently has write privileges on this repository, which should allow merging PRs. The merge was blocked because the expected GitHub Actions test runs hadn't passed, and the branch protections that we recently put in place require those test runs to pass. And the test runs didn't pass because they weren't running, which in turn was because the connected GitHub Actions workflow was present in the master branch but not in this branch. That's why the merge from master into this branch fixed things. We're likely to see the same effect with other open PRs that haven't had a merge from master recently. The fix should be the same for those PRs: merge master into the branch. |
|
@prabhuramachandran Re: admin privileges, @rahulporuri should be able to push that through on our side. |
Closes #1049
@opoplawski can you see if this fixes your build hang on Fedora 34? It at least "fixes" the hang for me on Windows. Plotting still doesn't work there (
mlab.test_plot3d()) despite pyvistaqt.BackgroundPlotter working okay, so there is more to fix. But this might already be good enough progress in that it will prevent totalpip installhangs for people using the VTK pre wheels. (They'll have a problem later when they try to actually plot, but that's at least easier to diagnose than an empty build log that took an hour of CI time).