Closed gtdang closed 7 months ago
Just documenting the failed tests for areas that need to be addressed yet.
FAILED hnn_core/tests/test_gui.py::test_gui_upload_params - traitlets.traitlets.TraitError: The 'value' trait of a FileUpload instance expected a tuple, not the dict...
FAILED hnn_core/tests/test_gui.py::test_gui_upload_data - traitlets.traitlets.TraitError: The 'value' trait of a FileUpload instance expected a tuple, not the dict...
FAILED hnn_core/tests/test_gui.py::test_gui_take_screenshots - AttributeError: 'Tab' object has no attribute '_titles'
FAILED hnn_core/tests/test_gui.py::test_gui_adaptive_spectrogram - AttributeError: 'Tab' object has no attribute '_titles'
This is pretty consistent with ipywidgets 8.x API changes to FileUpload and accessing collection widget attributes. Good to know!
Attention: 6 lines
in your changes are missing coverage. Please review.
Comparison is base (
1216038
) 91.34% compared to head (2d4bd76
) 91.35%. Report is 7 commits behind head on master.:exclamation: Current head 2d4bd76 differs from pull request most recent head 96e4a6a. Consider uploading reports for the commit 96e4a6a to get more accurate results
Files | Patch % | Lines |
---|---|---|
hnn_core/gui/_viz_manager.py | 83.87% | 5 Missing :warning: |
hnn_core/gui/gui.py | 92.30% | 1 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@ntolley Seems like the macOS tests are passing now. It's failing on docs and link check? Are you familiar with these errors?
New error for me as well @gtdang
Looks like it's a weird consequence of one of the version changes with the ipywidgets
?
@gtdang I did some digging and I think the answer is to add ipykernel
as an explicit dependency:
@ntolley adding ipykernel as a GUI dependency seemed to do the trick!
Now the docs build is timing out as it's taking longer than 10mins. Does this tend to happen? If so, we could try increasing the timeout limit from the default 10m.
@ntolley adding ipykernel as a GUI dependency seemed to do the trick!
Now the docs build is timing out as it's taking longer than 10mins. Does this tend to happen? If so, we could try increasing the timeout limit from the default 10m.
Hm never mind. It seems like the GUI captures are not rendering for creating the docs. I'll have to dig into why this is happening with the updated dependencies.
@ntolley I updated the circleci config file to bump the build timeout to 20mins. It passed this time however looking the logs the build took <2min... So I'm thinking that change wasn't necessary and the timeout was just some inconsistency/stability issue on the vm. But I guess it doesn't hurt to keep it at 20mins? I'm not sure what plan you are on and if runtime is limited.
But I guess it doesn't hurt to keep it at 20mins?
we need to find a solution to this ... @gtdang if you are willing to help that would be great. IMO, the GUI docs should be run separately. Currently running make html-noplot
also runs the GUI examples ... it's a bug
we need a dedicated machine for running the GUI docs because they are slow ... either on a paid plan on CircleCI or through the Brown infrastructure. Not sure how we'd link it to pull requests if we do it on the Brown infrastructure though.
But I guess it doesn't hurt to keep it at 20mins?
Let's merge this PR with the 10 min limit and try to find a long-term solution in separate pull requests
But I guess it doesn't hurt to keep it at 20mins?
Let's merge this PR with the 10 min limit and try to find a long-term solution in separate pull requests
We actually found another issue on Wednesday with the .capture()
method no longer rendering the GUI--an issue for the GUI docs/tutorials. After some digging it's related to https://github.com/jupyter-widgets/ipywidgets/issues/3871. So we'll need to find an alternative solution for capturing the state of the GUI and displaying it. Should we hold off on merging until that is figured out?
ughgh ... any workarounds for now?
maybe: https://html2canvas.hertzen.com/ ?
Not a bad option @jasmainak !
Truthfully I think it may be best to keep this PR focused on making the GUI functional, and in a separate one either fix the docs or test a new solution all together
Not a bad option @jasmainak !
Truthfully I think it may be best to keep this PR focused on making the GUI functional, and in a separate one either fix the docs or test a new solution all together
I agree. I think the docs solution will be in a separate PR. I'm not in love the the current implementation of the GUI tutorials (even when it is working properly). It's a bit confusing and resource intensive for for the end user. I think screenshots would more clear for the user, although it has the tradeoff being less a streamlined process for writers. There are ways to automate screen captures pyautogui. We can discuss more when we meet this week.
will pyautogui
require xvfb
or a virtual screen to make it work in the CIs? My concern is that exact coordinates for the CI may be difficult to guess ... but I'm open to anything that works and is a stable solution
will
pyautogui
requirexvfb
or a virtual screen to make it work in the CIs? My concern is that exact coordinates for the CI may be difficult to guess ... but I'm open to anything that works and is a stable solution
Yes it would be a 2 step process of first running the simulation and grabbing the screenshots and then creating the documentation using the images. It shouldn't need to be done every time we regenerate docs though, only if the UI changes. You can actually give it images of buttons/tabs for it to find the coordinates to click. The images should also have the mouse cursor to highlight context.
For html2canvas... I would be against adding a js runtime dependency just for screen caps. Also the execution would be pretty difficult because you would have to invoke the js after each cell execution... not at the end of the full notebook execution.
@jasmainak I'm good with going ahead and merging this PR as I think in the long term we are going to move to a screenshot method for tutorials (either automated or manual). In any case I think the purpose of this PR is achieved, the next one to work on is #651
Merge button is yours if you're good with its current state!
Thanks @gtdang ! Congratulations on your first merged PR 🥳 🍺 🎉
Completed
Though only to the scope of what was causes crashes with what I was testing (composition and simulation launch).There could be more areas were it should be updated.FileUpload
widget API_counter
trait ofFileUpload
.ipykernel
as a GUI dependency. ipywidgets removed its dependency of ipykernel to allow more flexibility to use other kernels. This might only need to be a test dependency?Remarks
I've only tested the using the GUI to launch the default simulation and adding some of the plots. Will need to test/search for more updates to the ipywidgets 8.x API. I suspect features like file-upload also need to be updated.Questions
FileUpload
button widget has a counter displayed showing the number of files uploaded. This counter cannot be disabled. (https://github.com/jupyter-widgets/ipywidgets/issues/2904) For the network connectivity and drives upload button, the current code always keeps this number at 0. Do we want to keep it that way or should it counter increment to 1 if the user loads a param file?Personally I don't think the counter is great for our application and wish it could be removed... I think the user does need to have some confirmation that a file has been loaded though. An informational display of what files have been loaded for network and drive would be nice. And this could even show that a default file has been loaded at the start.We decided to keep the counter always at 0.To do: