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

Widget screenshot in magicgui.md error #283

Closed
lucyleeow opened this issue Dec 2, 2023 · 47 comments · Fixed by #294
Closed

Widget screenshot in magicgui.md error #283

lucyleeow opened this issue Dec 2, 2023 · 47 comments · Fixed by #294
Labels
bug Something isn't working

Comments

@lucyleeow
Copy link
Collaborator

lucyleeow commented Dec 2, 2023

🐛 Bug

Widget screenshot in magicgui.md (in dev docs: https://napari.org/dev/guides/magicgui.html) looks wrong:

image

Not familiar with magicgui so not sure if this is due to our example or the screenshot tool?

@lucyleeow lucyleeow added the bug Something isn't working label Dec 2, 2023
@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

Could you put a link to webpage? Or it happens when build locally?

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Dec 3, 2023

It's here, in the dev docs:
https://napari.org/dev/guides/magicgui.html
Stable looks file.
Screenshot below also looks fine, so maybe it's just some glitch? Let me check a CircleCI preview.
Edit: Here's the same page on a CircleCI preview from a most recently updated PR:
https://output.circle-artifacts.com/output/job/6ed6c9c2-7037-4f5c-84e0-93d5c4d1472f/artifacts/0/docs/docs/_build/guides/magicgui.html
Looks fine, so I think it will be fixed by the next commit to main. Going to leave this open so we remember to double check it.

@lucyleeow
Copy link
Collaborator Author

Interesting. Maybe it's a browser problem, I'll check with chromium

@lucyleeow
Copy link
Collaborator Author

Whoops mis read. It looks fine on CI for me too so hopefully fixed by commit

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

I have merged new commit and problem still exists (image is regenerated).

@psobolewskiPhD
Copy link
Member

I think it would have to be merged to napari/docs/main?

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

the dev docs are generated on every merge to main of napari/napari repository.

@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Dec 3, 2023

Screenshot below also looks fine,

Which screenshot? Just to clarify you see the same problem in dev?

(dev vs stable)

Do we generate stable/dev differently? Maybe a different magicgui version (is causing the problem)?

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

We have not regenerated stable since the 0.4.18 release.

@psobolewskiPhD
Copy link
Member

Screenshot below also looks fine,

Which screenshot? Just to clarify you see the same problem in dev?

The napari generated screenshot just below the wierd one.
Also whats with the add caption here?

@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Dec 3, 2023

Also whats with the add caption here?

I know right?! I was very confused by this 😂

We have not regenerated stable since the 0.4.18 release.

The example does not use napari at all. Just magicgui, datetime and pathlib (thus maybe change to magicgui version or screenshotting tool??)

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

Also whats with the add caption here?

*Add caption here*

It is here in code. Some bug in during code review happens.

@lucyleeow
Copy link
Collaborator Author

Oh yeah I knew it was in the code. I did not know the intention but yes probably missed in code review.

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

I have found that we have two workflows for build docs:

https://github.com/napari/docs/blob/main/.github/workflows/deploy_docs.yml
and
https://github.com/napari/docs/blob/main/.github/workflows/build_docs.yml

The deploy one do not use constraints in make steep.

I think that we should use a single workflow with conditional deeploy steep.

@lucyleeow
Copy link
Collaborator Author

Does the code used:

from magicgui import magicgui
import datetime
import pathlib

@magicgui(
    call_button="Calculate",
    slider_float={"widget_type": "FloatSlider", 'max': 10},
    dropdown={"choices": ['first', 'second', 'third']},
)
def widget_demo(
    maybe: bool,
    some_int: int,
    spin_float=3.14159,
    slider_float=4.5,
    string="Text goes here",
    dropdown='first',
    date=datetime.datetime.now(),
    filename=pathlib.Path('/some/path.ext')
):
    ...

widget_demo.show()

work for others? It generates an empty widget for me (but I am likely to be running it wrong).

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

You need to launch event loop to have visible widget:

obraz

from qtpy.QtWidgets import QApplication

from magicgui import magicgui
import datetime
import pathlib

app = QApplication([])

@magicgui(
    call_button="Calculate",
    slider_float={"widget_type": "FloatSlider", 'max': 10},
    dropdown={"choices": ['first', 'second', 'third']},
)
def widget_demo(
    maybe: bool,
    some_int: int,
    spin_float=3.14159,
    slider_float=4.5,
    string="Text goes here",
    dropdown='first',
    date=datetime.datetime.now(),
    filename=pathlib.Path('/some/path.ext')
):
    ...

widget_demo.show()
app.exec()

@lucyleeow
Copy link
Collaborator Author

Ahhhh okay, I see we have:

def napari_scraper(block, block_vars, gallery_conf):

which does the app stuff.

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

But this iterates over napari main windowses. How should it discover the magicgui widget?

@lucyleeow
Copy link
Collaborator Author

I see we have:

# As much as possible, this file should be kept in sync with
# https://github.com/napari/napari/blob/main/.github/workflows/build_docs.yml

so there may be a reason we didn't/couldn't use the same workflow, @melissawm ?

Deploy uses constraints:

python -m pip install "napari-repo/[all]" -c "napari-repo/resources/constraints/constraints_py3.10_docs.txt"
python -m pip install -r docs/requirements.txt -c "napari-repo/resources/constraints/constraints_py3.10_docs.txt"

Build (used in PRs), just uses the requirements.txt file:

python -m pip install -r docs/requirements.txt

@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Dec 3, 2023

But this iterates over napari main windowses. How should it discover the magicgui widget?

I am not good with Qt stuff but maybe this will answer your question: https://sphinx-gallery.github.io/stable/advanced.html#write-a-custom-image-scraper

tl;dr for each 'code' block/section in the docs, images are scraped using napari_scraper. I think the code in the code block is executed and then the scraper code (is run to get any images resulting from the executing the code block).

@Czaki
Copy link
Contributor

Czaki commented Dec 3, 2023

so there may be a reason we didn't/couldn't use the same workflow, @melissawm ?

I'm author of this note. It is because when I was fixing build_docs workflows I miss deploy docs one.

Build also uses constraints but pass them using enviornment variable:

python -m pip install -r docs/requirements.txt
env:
PIP_CONSTRAINT: ${{ github.workspace }}/napari/resources/constraints/constraints_py3.10_docs.txt

Also make uses constraints

env:
GOOGLE_CALENDAR_ID: ${{ secrets.GOOGLE_CALENDAR_ID }}
GOOGLE_CALENDAR_API_KEY: ${{ secrets.GOOGLE_CALENDAR_API_KEY }}
PIP_CONSTRAINT: ${{ github.workspace }}/napari/resources/constraints/constraints_py3.10_docs.txt
with:
run: make -C docs docs

When deploy do not use this:

env:
GOOGLE_CALENDAR_ID: ${{ secrets.GOOGLE_CALENDAR_ID }}
GOOGLE_CALENDAR_API_KEY: ${{ secrets.GOOGLE_CALENDAR_API_KEY }}
with:
# the napari-docs repo is cloned into a docs/ folder, hence the
# invocation below. Locally, you should simply run make docs
run: make -C docs docs GALLERY_PATH=../examples/

@lucyleeow
Copy link
Collaborator Author

Thanks but I do not follow what PIP_CONSTRAINT does here:

env:
GOOGLE_CALENDAR_ID: ${{ secrets.GOOGLE_CALENDAR_ID }}
GOOGLE_CALENDAR_API_KEY: ${{ secrets.GOOGLE_CALENDAR_API_KEY }}
PIP_CONSTRAINT: ${{ github.workspace }}/napari/resources/constraints/constraints_py3.10_docs.txt

since in "Install Dependencies" step above we used the constraints?

I think that we should use a single workflow with conditional deploy steep.

I'll make another issue to see if we can use the same workflow for both.

@lucyleeow
Copy link
Collaborator Author

Ahhh i see we changed the way we scraped images right after 0.4.18 release: #207

@aganders3 could this be why the magicgui widget does not work? I guess since this scraper is specific for napari?

@aganders3
Copy link
Contributor

Eek I'm just catching up on this but yes, it's a possibility. I can take a look at this tomorrow.

@psobolewskiPhD
Copy link
Member

We use that scraper in the CircleCI docs which look fine so I dont think that's it. Note they use the update workflow like build_docs.

@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Dec 3, 2023

update workflow

What is the update workflow?

What is the difference in the environment between the "build docs" and "build and deploy"? So I can try to replicate locally. To me they both seem to use the same constraints..?

#283 (comment)

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Dec 3, 2023

Sorry I wasn't clear, this PR updated the build_docs and the circle ci workflows, but not the deploy one:
#266
The issue is that the make docs step has a pip install step in it:

docs/Makefile

Line 24 in de3c3e0

python -m pip install -qr $(current_dir)requirements.txt

So without the env variable, that can install different versions of stuff than the previous steps with constraints, which is what Grzegorz was pointing out.

@lucyleeow
Copy link
Collaborator Author

Ahhh I did not think the Makefile would have an install step!

This setup seems odd? Why do we need the "Install Dependencies" step in build_docs then? Just to print napari version and napari.layers.__doc__ ?

@aganders3
Copy link
Contributor

aganders3 commented Dec 4, 2023

I'm just poking around this morning. Docs look okay for me when built locally, but I will say I am a little confused still how that first magicgui (non-napari) image works at all.

Also whats with the add caption here?

This might be a red herring as it's actually just...in the doc?

*Add caption here*

@aganders3
Copy link
Contributor

aganders3 commented Dec 4, 2023

Okay now I am understanding more. This has nothing to do with the scraper because that is for the gallery only. These screenshots come from myst-nb, which renders the output of the cells in that file because they use the code-cell directive.

The first widget shows in the notebook because it provides _repr_png_.

The rest of the widgets are rendered more manually by combining two code blocks: one with remove-output (shows the code) and one with remove-input (shows a screenshot):

```{code-cell} python
:tags: [remove-output]
import napari
import numpy as np
from napari.layers import Image
@magicgui(image={'label': 'Pick an Image'})
def my_widget(image: Image):
...
viewer = napari.view_image(np.random.rand(64, 64), name="My Image")
viewer.window.add_dock_widget(my_widget)
```
*Note the widget at the bottom with "My Image" as the currently selected option*
```{code-cell} python
:tags: [remove-input]
from napari.utils import nbscreenshot
viewer.window._qt_window.resize(750, 550)
nbscreenshot(viewer, alt_text="A magicgui widget using an image layer parameter annotation")
```

It looks like maybe Qt was in some weird state here. Each screenshot sets its own size but in the output they successively shrink for some reason. edit: maybe adding some viewer.close() or napari.Viewer.close_all() (perhaps even in other files) would help?

@melissawm
Copy link
Member

I'm just poking around this morning. Docs look okay for me when built locally, but I will say I am a little confused still how that first magicgui (non-napari) image works at all.

Also whats with the add caption here?

This might be a red herring as it's actually just...in the doc?

*Add caption here*

We had a sprint for alt-text once and marked this for someone to write something since IIRC the screenshot for magicgui does not support alt-text out of the box. If someone can add a description that would be great 😄

@melissawm
Copy link
Member

I think some of this is vestigial? The idea was probably a simple make docs and when we have mono repo this probably worked. Now we need to install napari and then install the docs dependencies. Plus we have the constraints making sure nothing is broken by new versions of dependencies. I think for the CI workflows, we could also just switch from make docs

docs/Makefile

Line 35 in de3c3e0

docs: clean docs-install docs-build

to make clean docs-build and skip the extra install step inside the makefile.

To be honest, ideally I would love for the dependency install to be de-coupled from the docs build command. I understand the convenience but I feel like it's confusing to have them together. But that's a discussion for another time 😅

@melissawm
Copy link
Member

I see we have:

# As much as possible, this file should be kept in sync with
# https://github.com/napari/napari/blob/main/.github/workflows/build_docs.yml

so there may be a reason we didn't/couldn't use the same workflow, @melissawm ?

Deploy uses constraints:

python -m pip install "napari-repo/[all]" -c "napari-repo/resources/constraints/constraints_py3.10_docs.txt"
python -m pip install -r docs/requirements.txt -c "napari-repo/resources/constraints/constraints_py3.10_docs.txt"

Build (used in PRs), just uses the requirements.txt file:

python -m pip install -r docs/requirements.txt

The reason is that those two workflows were similar but one was updated and the other wasn't. So +1 to getting deploy_docs to be the same as build_docs!

psobolewskiPhD pushed a commit that referenced this issue Dec 4, 2023
# References and relevant issues
Possible f i x es #283

# Description
Add constraints as env variable to the build step of 'deploy_docs'
workflow, matching the 'build_docs' workflow.

(no further improvement of these workflows attempted in this PR, they
can be done later, see #284)
@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Dec 5, 2023

To be honest, ideally I would love for the dependency install to be de-coupled from the docs build command.

+1 here. I think it would be great if using make docs did not install things, as it may cause unintended consequences for the user if they did not know it included installation. I am not that experienced with docs but but I've never seen make docs install packages.

(I'll start an issue anyway so we can have a discussion)

@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Dec 5, 2023

maybe adding some viewer.close() or napari.Viewer.close_all() (perhaps even in other files) would help?

Yes it is probably safer to do something like this. The annoying part though is that we haven't been able to replicate this problem locally. I guess we will need to try installing from requirements as is/was in deploy_docs (via the makefile)

Edit: of note, the docs look fine in CI, so it's not a change in napari that has cause this.

@lucyleeow
Copy link
Collaborator Author

lucyleeow commented Dec 5, 2023

@melissawm do you know if there is a reason we use sphinx-gallery for code outputs in the gallery but myst-nb elsewhere in the doc? It would be nice to consistently use one tool?

Edit: I guess it would involve a lot of re-arranging to use sphinx gallery for non-gallery docs, especially as they use RST. Is myst-nb an option for gallery docs?

@psobolewskiPhD
Copy link
Member

So I'm pretty sure that the deploy workflow worked properly now that #290 was merged.
The deployment action finished on the GitHub.io repo.
And yet, the widget is still messed up like before:
https://napari.org/dev/guides/magicgui.html
But the preview docs on CircleCI look fine:
https://output.circle-artifacts.com/output/job/a6aac828-d441-45bc-871d-c0ae279fd1f2/artifacts/0/docs/docs/_build/guides/magicgui.html
Also, the zip artifact (GH runners build_docs) for #290 (https://github.com/napari/docs/actions/runs/7093485902)
has the same issue. But when I build locally, the image is fine.
So it seems like it's related to the GH runners? 🤔

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Dec 5, 2023

I spot two differences between GH and CircleCI (and my local build): we use Talleys QT action:

- uses: tlambert03/setup-qt-libs@v1

This action seems to run:
https://github.com/tlambert03/setup-qt-libs/blob/b66de97cf84c0684fa084b8487cfd4ae2c622f40/dist/index.js#L2982

While in CircleCI we install manually:

name: Install qt libs + xvfb
command: sudo apt-get update && sudo apt-get install -y xvfb libegl1 libdbus-1-3 libxkbcommon-x11-0 libxcb-icccm4 libxcb-image0 libxcb-keysyms1 libxcb-randr0 libxcb-render-util0 libxcb-xinerama0 libxcb-xinput0 libxcb-xfixes0 x11-utils

The difference appears that the Talley action also has libxcb-cursor0 libopengl0 libegl1-mesa while circleci has xvfb.
But looking at the log it looks like xvfb does pull libegl-mesa0 (note the 0, not sure it's important) but not the others :
https://app.circleci.com/pipelines/github/napari/docs/413/workflows/b7d5ed05-1315-4de4-8689-9ed1ee574100/jobs/403?invite=true#step-103-3065_45
(And locally obviously I have macOS stuff)
Then in GH workflow we use the @aganders3 headless run:
uses: aganders3/headless-gui@v1

while CircleCI uses xvfb:
xvfb-run --auto-servernum make docs

and locally I get the windows dancing.

@aganders3
Copy link
Contributor

aganders3 commented Dec 5, 2023

Then in GH workflow we use the @aganders3 headless run

I thought I was off the hook when we determined it wasn't the scraper 😆.

Buuuut - this action runs a tiling window manager by default, so maybe this could cause the shrinking screenshots. I don't really know how it would mess up that initial screenshot but who knows. So my previous suggestion of using viewer.close() might also work here. Alternatively the action can be configured by setting linux-pkgs and linux-setup, so maybe setting something like this would work:

      - name: Build Docs
        uses: aganders3/headless-gui@v1
        env:
          GOOGLE_CALENDAR_ID: ${{ secrets.GOOGLE_CALENDAR_ID }}
          GOOGLE_CALENDAR_API_KEY: ${{ secrets.GOOGLE_CALENDAR_API_KEY }}
          PIP_CONSTRAINT: ${{ github.workspace }}/napari/resources/constraints/constraints_py3.10_docs.txt
        with:
          run:  make -C docs docs
+          linux-pkgs: ""
+          linux-setup: ""

@aganders3
Copy link
Contributor

I may be reading it wrong but it also looks like the CircleCI build is using PySide and the GH-Actions build is using PyQt5 (from napari[all]).

@psobolewskiPhD
Copy link
Member

Good catch. I'm using pyqt6 locally, which is also fine, so a glitch in pyqt5 specifically is also possible?

@aganders3
Copy link
Contributor

Could still be specific to headless too. Also I can see there's a different code path for this rendering code in magicgui depending on PySide/PyQt. I'm otherwise running low on ideas 😬.

@aganders3
Copy link
Contributor

I tried PySide in my PR and get no image now but an exception instead (in the docs output):

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
File /opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/IPython/core/formatters.py:344, in BaseFormatter.__call__(self, obj)
    342     method = get_real_method(obj, self.print_method)
    343     if method is not None:
--> 344         return method()
    345     return None
    346 else:

File /opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/magicgui/widgets/bases/_widget.py:403, in Widget._repr_png_(self)
    397     print(
    398         "(For a nicer magicgui widget representation in "
    399         "Jupyter, please `pip install imageio`)"
    400     )
    401     return None
--> 403 rendered = self.render()
    404 if rendered is not None:
    405     with BytesIO() as file_obj:

File /opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/magicgui/widgets/bases/_widget.py:384, in Widget.render(self)
    382 def render(self) -> np.ndarray:
    383     """Return an RGBA (MxNx4) numpy array bitmap of the rendered widget."""
--> 384     return self._widget._mgui_render()

File /opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/magicgui/backends/_qtpy/widgets.py:187, in QBaseWidget._mgui_render(self)
    185 h, w, c = img.height(), img.width(), 4
    186 if qtpy.API_NAME.startswith("PySide"):
--> 187     arr = np.array(bits).reshape(h, w, c)
    188 else:
    189     bits.setsize(h * w * c)

ValueError: cannot reshape array of size 204160 into shape (290,352,4)

Here's the run: https://github.com/napari/docs/actions/runs/7108661085

@aganders3
Copy link
Contributor

aganders3 commented Dec 6, 2023

Okay I have another theory now that this is related to the bit-depth of the xvfb (virtual) screen.

In my headless-gui action the screen is set here:
https://github.com/aganders3/headless-gui/blob/5124e2e44f3012512071e525269b50d0cadc3ef7/src/start-xvfb.bash#L25-L26

In CircleCI it just uses xvfb-run with defaults, which I think is 640x480x8 don't know.

I will see if I can figure out a reasonable way to test this theory tomorrow 😓.

@psobolewskiPhD
Copy link
Member

Ooo! Bit depth difference could maybe explain the funky colors?
https://manpages.ubuntu.com/manpages/xenial/man1/xvfb-run.1.html
suggests the default is -screen 0 640x480x8

psobolewskiPhD added a commit that referenced this issue Dec 15, 2023
# References and relevant issues
~~Hopefully f i x e s~~ Pretty sure now this fixes both #283 and #285 

# Description
This configures the
[headless-gui](https://github.com/aganders3/headless-gui) action to not
run the tiling window manager it uses by default. My thinking is the
tiling behavior in the window manager is causing issues with certain
screenshots. See
#283 (comment) for
more details.

This also updates the action to a new version with a more common default
screen size. This change in bit-depth fixes the messed up magicgui
screenshot.

Edit: I'm still not _totally_ clear why the stable docs would look okay
but I think this is worth trying.

---------

Co-authored-by: Peter Sobolewski <[email protected]>
@psobolewskiPhD psobolewskiPhD linked a pull request Dec 16, 2023 that will close this issue
@psobolewskiPhD
Copy link
Member

Fixed by #294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants