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

Improve makefile #41

Merged
merged 4 commits into from Feb 5, 2023
Merged

Improve makefile #41

merged 4 commits into from Feb 5, 2023

Conversation

melissawm
Copy link
Member

Description

Makes building paths more generic and allows building from a local napari clone folder.

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

Related to, but does not fix, #5222

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added documentation Improvements or additions to documentation task labels Nov 5, 2022
@melissawm melissawm marked this pull request as draft December 13, 2022 19:04
@melissawm melissawm removed the documentation Improvements or additions to documentation label Dec 13, 2022
@melissawm melissawm marked this pull request as ready for review December 13, 2022 19:55
@github-actions github-actions bot added documentation Improvements or additions to documentation and removed task labels Dec 13, 2022
@melissawm melissawm added task and removed documentation Improvements or additions to documentation labels Dec 13, 2022
@DragaDoncila
Copy link
Contributor

@melissawm this looks great to me in essence, but I can't test it locally as my docs build doesn't work. I'm no expert with make. I run

make docs

and I get

(napari-edit) docs $ make docs          
echo clean
clean
echo /Users/ddoncilapop/CZI/docs/
/Users/ddoncilapop/CZI/docs/
rm -rf /Users/ddoncilapop/CZI/docs/docs/_build/
rm -rf /Users/ddoncilapop/CZI/docs/docs/api/napari*.rst
rm -rf /Users/ddoncilapop/CZI/docs/docs/gallery/*
rm -rf /Users/ddoncilapop/CZI/docs/docs/_tags
python -m pip install -qr /Users/ddoncilapop/CZI/docs/requirements.txt
python /Users/ddoncilapop/CZI/docs/docs/_scripts/prep_docs.py
Cloning into '/Users/ddoncilapop/CZI/docs/npe2'...
remote: Enumerating objects: 2135, done.
remote: Counting objects: 100% (49/49), done.
remote: Compressing objects: 100% (47/47), done.
remote: Total 2135 (delta 19), reused 6 (delta 2), pack-reused 2086
Receiving objects: 100% (2135/2135), 581.13 KiB | 7.00 MiB/s, done.
Resolving deltas: 100% (1420/1420), done.
Traceback (most recent call last):
  File "/Users/ddoncilapop/CZI/docs/npe2/_docs/render.py", line 20, in <module>
    from npe2 import PluginManager, PluginManifest
ImportError: cannot import name 'PluginManager' from 'npe2' (unknown location)
Traceback (most recent call last):
  File "/Users/ddoncilapop/CZI/docs/docs/_scripts/prep_docs.py", line 29, in <module>
    main()
  File "/Users/ddoncilapop/CZI/docs/docs/_scripts/prep_docs.py", line 23, in main
    prep_npe2()
  File "/Users/ddoncilapop/CZI/docs/docs/_scripts/prep_docs.py", line 18, in prep_npe2
    check_call([sys.executable, f"{NPE}/_docs/render.py", DOCS / 'plugins'])
  File "/opt/miniconda3/envs/napari-edit/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/opt/miniconda3/envs/napari-edit/bin/python', '/Users/ddoncilapop/CZI/docs/npe2/_docs/render.py', PosixPath('/Users/ddoncilapop/CZI/docs/docs/plugins')]' returned non-zero exit status 1.
make: *** [docs-build] Error 1

I'm confident this probably has nothing to do with your PR, but I can't test it really... Maybe @psobolewskiPhD you can test this locally? I know you've been playing with docs a bit.

@psobolewskiPhD
Copy link
Member

I've never built the docs locally 👀
always using the markdown preview and PR build docs action 👀
I'm traveling so not sure when I will get to try to do this.

@Czaki
Copy link
Contributor

Czaki commented Jan 5, 2023

I'm traveling so not sure when I will get to try to do this.

When you play with styles, global changes, structural changes, etc. It is much faster to build it locally than wait on CI.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Jan 5, 2023

@DragaDoncila I got similar errors in a fresh env with just the requirements.txt following https://napari.org/dev/developers/documentation/index.html
I couldn't get main or this PR to build until I installed napari into the env (also once again not being able to install "napari[dev]" proves annoying on my arm64 mac, see also napari/napari#5438 )

@melissawm should the instructions here be amended?
https://napari.org/dev/developers/documentation/index.html#prerequisites
They seem to suggest it's not needed to install napari.

Also, the rm -rf part didn't work due to a permissions error so I manually did it.

I'm traveling so not sure when I will get to try to do this.

When you play with styles, global changes, structural changes, etc. It is much faster to build it locally than wait on CI.

@Czaki Much faster he says... I don't know about that 🤣
Also all the popping up of naparis makes my computer non-functional.
Maybe if it was possible to skip the examples? 🤷‍♂️

But anyhow, once I had napari installed in the env and this PR, everything seemed to build.

@melissawm
Copy link
Member Author

Ok - I'm not sure if this is the case for you folks but it seems related.

$ conda create --name napari-310 python=3.10
$ conda activate napari-310
$ python -m pip install -e ".[all]"
$ conda list
...
ipython                   8.7.0                    pypi_0    pypi
...
$ napari  # works fine
$ ipython
Python 3.10.8 (main, Nov  1 2022, 14:18:21) [GCC 12.2.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.7.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import napari
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
Cell In[1], line 1
----> 1 import napari

ModuleNotFoundError: No module named 'napari'

In [2]:

Now the second part: if in this same environment I do

$ conda install ipython

now I have

$ conda list
...
ipython                   8.7.0              pyh41d4057_0    conda-forge
...

and now, import napari works. -> So now the docs should build, but still they don't, because I still get a ModuleNotFoundError: No module named 'napari'. It looks like this came about with Python 3.10, as I had never observed this before.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Jan 5, 2023

Actually, yeah that was on 3.10.
I'll try with 3.9... brb

Edit: no luck.
Fresh env (3.9) from conda.
then python -m pip install -r requirements.txt
then make docs
gives:

echo clean
clean
rm -rf docs/_build/
rm -rf docs/api/napari*.rst
rm -rf docs/_tags
rm -rf docs/gallery/
python -m pip install -qr requirements.txt
python docs/_scripts/prep_docs.py
Cloning into 'npe2'...
remote: Enumerating objects: 2135, done.
remote: Counting objects: 100% (49/49), done.
remote: Compressing objects: 100% (47/47), done.
remote: Total 2135 (delta 19), reused 6 (delta 2), pack-reused 2086
Receiving objects: 100% (2135/2135), 581.13 KiB | 445.00 KiB/s, done.
Resolving deltas: 100% (1420/1420), done.
Traceback (most recent call last):
  File "/Users/piotrsobolewski/Dev/napari-docs/npe2/_docs/render.py", line 18, in <module>
    from magicgui._magicgui import MagicFactory
ModuleNotFoundError: No module named 'magicgui'
Traceback (most recent call last):
  File "/Users/piotrsobolewski/Dev/napari-docs/docs/_scripts/prep_docs.py", line 29, in <module>
    main()
  File "/Users/piotrsobolewski/Dev/napari-docs/docs/_scripts/prep_docs.py", line 23, in main
    prep_npe2()
  File "/Users/piotrsobolewski/Dev/napari-docs/docs/_scripts/prep_docs.py", line 18, in prep_npe2
    check_call([sys.executable, "npe2/_docs/render.py", DOCS / 'plugins'])
  File "/Users/piotrsobolewski/Dev/miniforge3/envs/nap-docs/lib/python3.9/subprocess.py", line 373, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/Users/piotrsobolewski/Dev/miniforge3/envs/nap-docs/bin/python', 'npe2/_docs/render.py', PosixPath('/Users/piotrsobolewski/Dev/napari-docs/docs/plugins')]' returned non-zero exit status 1.
make: *** [docs-build] Error 1

@psobolewskiPhD
Copy link
Member

I tried installing magicgui, then it complained about npe2, so I did that, then it complained about Qt.
Once I installed pyqt and napari, make docs worked, with some errors due to not having [dev], but it worked.
So I think the instructions need to clarified that napari has to be installed, unless I'm really missing something.

@melissawm
Copy link
Member Author

Oh yes. If you are building the docs, you need to install napari[dev] in the environment first. In any case, let me try and debug this and maybe amend the docs.

@psobolewskiPhD
Copy link
Member

OK I think the 2nd point in this section:
https://napari.org/dev/developers/documentation/index.html#prerequisites

If you wish to add/amend documentation that does contain code...

should just start with "To build docs locally or if you wish ..."

Makes building paths more generic and allows building from a local napari clone folder.
@melissawm
Copy link
Member Author

melissawm commented Jan 5, 2023

So right now it reads

If you wish to add/amend documentation that does contain code, you will also require a clean conda environment with napari docs dependencies installed, but with a development installation of napari.

If I add a link here to the development installation instructions, is that clear enough?

Nevermind, I see that this should be added to the first item too!

@psobolewskiPhD
Copy link
Member

@melissawm Maybe straying off topic, but what do you think of going back to making a napari[docs] that's a bit lighter than the current [dev] which includes everything for napari testing (torch, tensorstore)? We could try to make a what's needed just for examples to build?

Also adds napari[dev] as a docs dependency, to allow building the docs without installing the napari development version first.
@github-actions github-actions bot added documentation Improvements or additions to documentation and removed task labels Jan 5, 2023
@melissawm
Copy link
Member Author

I think I went the other way here, which is: if you are only focused on the docs, you can install the latest napari[dev] from pypi and the docs will build, albeit maybe not with the latest changes from main. Let me know what you prefer - I think for now all the docs dependencies were actually removed from the main setup.cfg which is why I would hesitate to propose adding them again.

@psobolewskiPhD
Copy link
Member

Yeah, I get that.
[dev] is just really heavy with a lot of stuff not needed for docs because it pulls [testing].
But yes, [docs] would need to be re-added—which is doable (but for it to work of pypi would take a release).

@melissawm
Copy link
Member Author

Hm ok, I was wrong here - as it is now, you actually do need to clone the main napari folder to build the docs because of the example gallery. I will work some more on this.

Comment on lines +28 to +31
docs-xvfb:
python $(docs_dir)/_scripts/prep_docs.py
NAPARI_APPLICATION_IPY_INTERACTIVE=0 xvfb-run --auto-servernum sphinx-build -b html docs/ docs/_build -D sphinx_gallery_conf.examples_dirs=$(GALLERY_PATH)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psobolewskiPhD this command, if you have xvfb installed, will allow you to build the docs without seeing all of the napari windows 😍 Thanks to @dalthviz for the tip!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what that is/means at the moment, but will try to look into if it's a thing on mac!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xvfb is Linux specific.

@melissawm
Copy link
Member Author

Ok - I think the napari[docs] decision will have to be made later, but for now this completes the explanation for the current status of the requirements and installation instructions.

I have also added a docs-xvfb target in the makefile that may be useful for building locally for those who have xvfb installed.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Jan 27, 2023

Ok, I followed the directions on the new Contributing Docs page and everything built correctly!
🎉

I'm not sure how to use the xvbf thing. I tried:
make docs-xvbf
and that gave:
make: *** No rule to make target docs-xvbf'. Stop.`
I sort of assumed it would fail, because I probably need to install something, but this wasn't the fail I was expecting?

@melissawm
Copy link
Member Author

melissawm commented Jan 27, 2023

Maybe just a typo? it is make docs-xvfb

However, I do have the xorg-server-xvfb package installed in my Manjaro machine, so I think for a Mac this may be a little bit more effort - see for example this reddit thread

EDIT: Looks like this is the best option https://www.xquartz.org/Developer-Info.html

@psobolewskiPhD
Copy link
Member

Maybe just a typo? it is make docs-xvfb

Christ, sorry. I guess I've had one drink too many for being detail oriented!
it fails with /bin/sh: xvfb-run: command not found which would be expected.
I'll take a look at that link, but think this is good to go.

@@ -6,6 +6,7 @@ sphinx-favicon
sphinx-gallery
sphinx_autodoc_typehints==1.12.0
myst-nb
napari[dev]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With napari/napari#5438 I wonder if this is sufficient—in my experience the viewer does open, so I think it does need Qt, while now napari[dev] doesn't auto-install a Qt backend.
So for the live 0.4.17 behavior, using pyqt5, the default, this should be napari[pyqt,dev], as per:
#78

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, the just edited in this PR contribution stuff already says to install dev napari explicitly:

- a development installation of napari. Follow the [contributor guide](dev-installation) for details on how to do this;

That guide explains how [dev] works.

So I think we should just remove this line, because if you leave as is, then no QT backend, so stuff won't work. If you make it [pyqt,dev] then it only works when pypi wheel of pyqt is available—not on my mac for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we use these requirements in any place? If no then maybe just remove it and add docs extra to the napari setup.cfg?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be pro on a napari[docs], as I noted above:
#41 (comment)
Basically, [dev] installs [testing] which is pretty heavy. For docs I think we just need the examples to build.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100%, napari[docs] is the way

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe for now we remove it here, so this can be merged, then make napari PR and then make a new PR here to adjust the copy and the requirements.txt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can open a follow up as soon as this is merged. Let me remove this line here then

@psobolewskiPhD
Copy link
Member

Ok, so napari[dev] is removed now—but the instructions say to install it.
The plan now would be to merge this as is and then make napari PR for [docs] and update the docs again, as needed.
Sound good to everyone? @Czaki @jni ?

Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok

@melissawm
Copy link
Member Author

Oh sorry - I missed that. Let me rephrase.

@melissawm
Copy link
Member Author

Ok - I see what happened. I confused what was already merged and what is still in PRs.

#72 fixes the dev installation instructions, so if that is merged first, this PR will be just fine. Maybe we can do that?

@psobolewskiPhD
Copy link
Member

#72 has a merge conflict, I will take a look this evening and merge.

@psobolewskiPhD
Copy link
Member

#72 is merged—thanks for fixing the conflicts ❤️
I think this is good to go, but maybe best to wait till tomorrow to merge?

@psobolewskiPhD psobolewskiPhD merged commit d21a63e into napari:main Feb 5, 2023
@psobolewskiPhD
Copy link
Member

Done! on the napari[docs] side, I'm mimic the current [dev], so not include a Qt backend—we can expect someone doing dev to follow instructions? Just need to figure out the set of dependencies needed for examples.

@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone Oct 24, 2023
@Czaki Czaki modified the milestones: 0.5.0, 0.4.19 Oct 31, 2023
@melissawm melissawm deleted the improve-makefile branch January 9, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants