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

Update requirement sphinx-book-theme to ~=1.0.0 #1953

Merged
merged 2 commits into from Mar 7, 2023

Conversation

paugier
Copy link
Contributor

@paugier paugier commented Mar 2, 2023

It would be great to be able to use sphinx-book-theme 1.0.0!

See #1842 (comment)

@paugier
Copy link
Contributor Author

paugier commented Mar 2, 2023

"~=1" does not work. Trying the more standard "~=1.0.0"

@fperez
Copy link

fperez commented Mar 3, 2023

My understanding is that in general, for releasing conda packages it's best to not depend on RCs and instead to have not only all dependencies be unpinned, but also to point to final releases (I'm not an expert in the topic, so others can correct this if I'm missing something).

Under that assumption, then big 👍 for this - I'd much rather deploy jbook once this is merged, as it will remove the last rc-only pin and thus make deployments a lot easier (and we may also get a clean conda package to go along :)

@paugier
Copy link
Contributor Author

paugier commented Mar 3, 2023

Tests test_build.py::test_toc_numbered are failing. It might be a real bug.

The produced file contained:

    <input class="toctree-checkbox" id="toctree-checkbox-1" name="toctree-checkbox-1" type="checkbox"/>
    <label class="toctree-toggle" for="toctree-checkbox-1">
     <i class="fa-solid fa-chevron-down">
     </i>
    </label>
    <ul>
     <li class="toctree-l2">
      <a class="reference internal" href="subfolder/asubpage.html">
       Asubpage
      </a>
     </li>
    </ul>

and the saved files contain

    <input class="toctree-checkbox" id="toctree-checkbox-1" name="toctree-checkbox-1" type="checkbox">
     <label class="toctree-toggle" for="toctree-checkbox-1">
      <i class="fa-solid fa-chevron-down">
      </i>
     </label>
     <ul>
      <li class="toctree-l2">
       <a class="reference internal" href="subfolder/asubpage.html">
        Asubpage
       </a>
      </li>
     </ul>
    </input>

The difference is the / at the end of <input .../> and the missing final </input>.

I could change the saved file to fix the CI but is it the right thing to do?

@choldgraf
Copy link
Member

Maybe the simplest syntax to use would be >=1, <2? I still don't quite understand how ~= works but have using it because it's been convention in this org.

I do suspect some of the regression tests will need to be updated since the HTML structure changed. We will also need to update some of the documentation as it's now out of date I think.

@chrisjsewell
Copy link
Member

~=1 is literally anything, ~=1.1 is anything 1.x greater or equal to 1.1, ~=1.1.1 is anything 1.1.x greater or equal to 1.1.1

@chrisjsewell
Copy link
Member

t's best to not depend on RCs

For sure this 👍

instead to have not only all dependencies be unpinned, but also to point to final releases

I would suggest essentially this comes down to, whether you feel jupyter-book is responsible for stability or the user is.
Obviously if jupyter-book, and its dependencies, have no pins, then there is a high likelihood that at some point creating an environment via pip install jupyter-book or conda install jupyter-book will be broken, i.e. when a dependency releases a breaking change; jupyter-book will be inherently unstable

If it is changed, I would suggest it should be made very clear, in places like https://jupyterbook.org/en/stable/start/overview.html#install-jupyter-book, that it is the users' responsibility to create e.g. lock files, to ensure their book builds are stable

But anyhow, I'll leave you guys to it 😄

@chrisjsewell
Copy link
Member

(And yeh, there is also this related question of whether people should be using the same python environment for jupyter-book and jupyter kernels; #1898 (comment))

@fperez
Copy link

fperez commented Mar 4, 2023

I would suggest essentially this comes down to, whether you feel jupyter-book is responsible for stability or the user is.
Obviously if jupyter-book, and its dependencies, have no pins, then there is a high likelihood that at some point creating an environment via pip install jupyter-book or conda install jupyter-book will be broken, i.e. when a dependency releases a breaking change; jupyter-book will be inherently unstable

Excellent question @chrisjsewell, thanks - thinking about it this way gives me something to discuss with our Stat 159 students - pinging @facusapienza21 so we don't forget :)

My take on this is that libraries should be flexible in leaving as few pins as possible, so as to allow packages to be more easily combined in new ways, even if some of them prove to have problems. But users should be stricter in their own workflows, and use environments with more explicit control of at least the key libraries they rely on.

So I'd vote for jbook (and similar) to minimize pins/constraints as much as possible. keeping it to avoiding things known to simply not work together, but particularly not constraining against future evolution. And users should be the ones specifying their working versions for a given project.

Yes, this does leave open the door for users to occasionally find a combination that packages that in practice doesn't work, but I think that's easier to handle, whereas the "environment simply can't be installed" tends to be a hard wall that they can't get past.

@chrisjsewell
Copy link
Member

chrisjsewell commented Mar 4, 2023

My take on this is that libraries should be flexible in leaving as few pins as possible

I would note, that perhaps the original intention of jupyter-book was to be an application and not a library, i.e. then it would hard pin everything.
The main "impediment" to this is probably the use of kernels, discussed in #1898 (comment)
If it is deemed a library, then yeh I feel it should just be explicitly documented as such

The other thing to note is that, complimentary to relaxed pinning etc, is just to remove dependencies.
This was one of the goals in myst-nb, moving from v0.13.2:

    docutils>=0.15,<0.18
    importlib_metadata
    ipython
    ipywidgets>=7.0.0,<8
    jupyter-cache~=0.4.1
    jupyter_sphinx~=0.3.2
    myst-parser~=0.15.2
    nbconvert>=5.6,<7
    nbformat~=5.0
    pyyaml
    sphinx>=3.1,<5
    sphinx-togglebutton~=0.3.0

to current:

    importlib_metadata
    ipython
    jupyter-cache~=0.5.0
    nbclient
    myst-parser~=0.18.0
    nbformat~=5.0
    pyyaml
    sphinx>=4,<6
    typing-extensions

e.g. there will no longer be any nbconvert/ipywidgets clashes, because jupyter-book no longer depends on them 😄

@codecov
Copy link

codecov bot commented Mar 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (3bcc1e9) 91.42% compared to head (0b0e69a) 91.42%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1953   +/-   ##
=======================================
  Coverage   91.42%   91.42%           
=======================================
  Files           7        7           
  Lines         688      688           
=======================================
  Hits          629      629           
  Misses         59       59           
Flag Coverage Δ
pytests 91.42% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@paugier paugier force-pushed the update-sphinx-book-theme branch 2 times, most recently from 0e6214f to d665089 Compare March 5, 2023 20:16
@paugier paugier changed the title Loose the restriction on sphinx-book-theme versions (~=1) Update requirement sphinx-book-theme to ~=1.0.0 Mar 5, 2023
@paugier
Copy link
Contributor Author

paugier commented Mar 5, 2023

CI is green with sphinx-book-theme~=1.0.0. I kept ~=1.0.0 because I like the property of the Jupyter Book application that when it is installed with pip install jupyterbook in a clean venv, one is sure to get a consistent tested environment. At least it is better than sphinx-book-theme==0.4.0rc1.

I have no idea where and how to update the documentation 🙂

@mmcky
Copy link
Member

mmcky commented Mar 6, 2023

thanks everyone -- I completely agree re: not typically using rc in pins. I released 0.14 with sphinx-book-theme==0.4.0rc1 to get it released as I knew a number of folks were needing to use it. The sphinx-book-theme>=1.0.0 wasn't quite ready at the time and builds were failing when I tagged it.

--

thanks @paugier for opening this repin and commenting on the changes

The difference is the / at the end of <input .../> and the missing final .

I am looking at the updated fixtures and it seems they are all just dropping </input> (as per the comment above).

Could someone with better html knowledge let me know if that makes sense given the only change is an upgrade to sphinx-book-theme. Cheers. (cc: @choldgraf, @AakashGfude )

I'd like to push this forward -- and have a look at the latest myst-parser upgrades as well.

@mmcky
Copy link
Member

mmcky commented Mar 6, 2023

ah just re-read @choldgraf feedback

I do suspect some of the regression tests will need to be updated since the HTML structure changed.

  • We will also need to update some of the documentation as it's now out of date I think.

I agree with this -- but we can do this as part of the next release cycle for 0.14.1

@mmcky
Copy link
Member

mmcky commented Mar 6, 2023

but if someone can verify the </input> is expected that would be great 🙏

@AakashGfude
Copy link
Member

@mmcky yes, the <input /> tag is better and follows the HTML best practices. The <input></input> was suss. Specially label and everything wrapped around it.
So, +1 from my side for <input />

Copy link
Member

@mmcky mmcky left a comment

Choose a reason for hiding this comment

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

thanks for reviewing the html @AakashGfude -- I will go ahead and merge this with a note to update the docs.

@mmcky mmcky merged commit c0d3d0c into executablebooks:master Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants