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

UPGRADE: myst-parser 1.0 #479

Merged
merged 25 commits into from
Jun 30, 2023
Merged

Conversation

aleivag
Copy link
Contributor

@aleivag aleivag commented Mar 18, 2023

This updates the myst-parser dependency to ~1.0

The changes are simple:

  • Update imports
  • Deprecate DOCUTILS_EXCLUDED_ARGS and implemented the omit: [docutils]
  • Black also did some formatting

I know very little about how this uses myst-parser all i know is that my jupyter-book compiles and uses latest features of myst parser using this. the doc generator seems broken on master , so i cant really tell if this is me or just broken on base rev, please advice !

@aleivag aleivag force-pushed the upgrade-to-1 branch 2 times, most recently from 83f264b to 2f77516 Compare March 19, 2023 00:34
@aleivag aleivag marked this pull request as ready for review March 19, 2023 15:34
@mgaitan
Copy link

mgaitan commented Apr 4, 2023

I've tried this. It forced to downgrade sphinx from 6.1.0 to 5.3.0. why ?

and I got this error

# Sphinx version: 5.3.0
# Python version: 3.10.6 (CPython)
# Docutils version: 0.19 
# Jinja2 version: 3.0.3
# Last messages:

# Loaded extensions:
Traceback (most recent call last):
  File "/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/sphinx/cmd/build.py", line 276, in build_main
    app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
  File "/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/sphinx/application.py", line 223, in __init__
    self.setup_extension(extension)
  File "/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/sphinx/application.py", line 398, in setup_extension
    self.registry.load_extension(self, extname)
  File "/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/sphinx/registry.py", line 472, in load_extension
    metadata = setup(app)
  File "/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/myst_nb/__init__.py", line 8, in setup
    from .sphinx_ext import sphinx_setup
  File "/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/myst_nb/sphinx_ext.py", line 23, in <module>
    from myst_nb.sphinx_ import (
  File "/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/myst_nb/sphinx_.py", line 16, in <module>
    from myst_parser.mdit_to_docutils.sphinx_ import SphinxRenderer, create_warning
ImportError: cannot import name 'create_warning' from 'myst_parser.mdit_to_docutils.sphinx_' (/home/tin/.virtualenvs/shiphero/lib/python3.10/site-packages/myst_parser/mdit_to_docutils/sphinx_.py)

@aleivag
Copy link
Contributor Author

aleivag commented Apr 4, 2023

hi, what did you did to get this error (how do i reproduce)?

I've tried this. It forced to downgrade sphinx from 6.1.0 to 5.3.0. why ?

Don't know, nothing in the PR sets the version of sphinx, so i guess is within the allowed versions , if you give me the command you used i'm happy to check

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 52.00% and project coverage change: -0.63 ⚠️

Comparison is base (5d97244) 81.47% compared to head (3d0a6de) 80.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
- Coverage   81.47%   80.84%   -0.63%     
==========================================
  Files          29       30       +1     
  Lines        2618     2663      +45     
==========================================
+ Hits         2133     2153      +20     
- Misses        485      510      +25     
Flag Coverage Δ
pytests 80.84% <52.00%> (-0.63%) ⬇️

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

Impacted Files Coverage Δ
myst_nb/ext/glue/directives.py 68.46% <ø> (ø)
myst_nb/warnings_.py 40.47% <40.47%> (ø)
myst_nb/core/render.py 85.02% <50.00%> (-0.04%) ⬇️
myst_nb/core/config.py 78.12% <60.00%> (+0.07%) ⬆️
myst_nb/sphinx_.py 90.10% <66.66%> (+0.03%) ⬆️
myst_nb/docutils_.py 73.96% <71.42%> (ø)
myst_nb/__init__.py 71.42% <100.00%> (ø)
myst_nb/core/read.py 84.09% <100.00%> (+0.47%) ⬆️

... and 3 files with indirect coverage changes

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

@agoose77
Copy link
Collaborator

@aleivag thanks for this contribution! We'll get around to reviewing it soon, and keep you updated :)

@choldgraf choldgraf changed the title upgrade myst-parser dep to 1.0 UPGRADE: myst-parser 1.0 Apr 21, 2023
@choldgraf
Copy link
Member

I took a look at the CI jobs and noticed a few errors:

mypy errors in our pre-commit job

Apparently MyST-parser introduced a new way to construct create_warnings and it is checking for a particular warning type. The myst-parser warnings object is here:

https://github.com/executablebooks/MyST-Parser/blob/a784880c7fbd893566087b83deec081376740e2c/myst_parser/warnings_.py#L10-L26

Here's the error:

myst_nb/core/render.py:99: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]
myst_nb/core/render.py:225: error: Unexpected keyword argument "wtype" for "create_warning" of "DocutilsRenderer"  [call-arg]
/home/runner/.cache/pre-commit/repofub48hzv/py_env-python3.9/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/base.py:162: note: "create_warning" of "DocutilsRenderer" defined here
myst_nb/core/render.py:225: error: Unexpected keyword argument "wtype" for "create_warning" of "DocutilsRenderer"  [call-arg]
myst_nb/core/render.py:228: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]
/home/runner/.cache/pre-commit/repofub48hzv/py_env-python3.9/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/base.py:162: note: "create_warning" of "DocutilsRenderer" defined here
myst_nb/core/render.py:228: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]
myst_nb/core/render.py:979: error: Unexpected keyword argument "wtype" for "create_warning" of "DocutilsRenderer"  [call-arg]
/home/runner/.cache/pre-commit/repofub48hzv/py_env-python3.9/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/base.py:162: note: "create_warning" of "DocutilsRenderer" defined here
myst_nb/core/render.py:979: error: Unexpected keyword argument "wtype" for "create_warning" of "DocutilsRenderer"  [call-arg]
myst_nb/core/render.py:983: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]
/home/runner/.cache/pre-commit/repofub48hzv/py_env-python3.9/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/base.py:162: note: "create_warning" of "DocutilsRenderer" defined here
myst_nb/core/render.py:983: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]
myst_nb/sphinx_.py:305: error: Unexpected keyword argument "wtype" for "create_warning" of "DocutilsRenderer"  [call-arg]
/home/runner/.cache/pre-commit/repofub48hzv/py_env-python3.9/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/base.py:162: note: "create_warning" of "DocutilsRenderer" defined here
myst_nb/sphinx_.py:310: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]
myst_nb/docutils_.py:306: error: Unexpected keyword argument "wtype" for "create_warning" of "DocutilsRenderer"  [call-arg]
/home/runner/.cache/pre-commit/repofub48hzv/py_env-python3.9/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/base.py:162: note: "create_warning" of "DocutilsRenderer" defined here
myst_nb/docutils_.py:312: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]
myst_nb/docutils_.py:337: error: Unexpected keyword argument "wtype" for "create_warning" of "DocutilsRenderer"  [call-arg]
/home/runner/.cache/pre-commit/repofub48hzv/py_env-python3.9/lib/python3.9/site-packages/myst_parser/mdit_to_docutils/base.py:162: note: "create_warning" of "DocutilsRenderer" defined here
myst_nb/docutils_.py:342: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "str"; expected "MystWarnings"  [arg-type]

Regression test breakages

This one is probably simple to fix, there are some regression test changes detected:

For example:

-        <footnote_reference auto="1" ids="footnote-reference-1" refid="a">
+        <footnote_reference auto="1" ids="id1" refid="a">
             1
          
-        <footnote_reference auto="1" ids="footnote-reference-2" refid="b">
+        <footnote_reference auto="1" ids="id2" refid="b">
             2

To fix these you generally just need to delete the corresponding regression test file, and then re-run the tests.

@kloczek
Copy link

kloczek commented May 2, 2023

I've been trying to use that PR on top of 0.17.2 but looks like PR needs to be rediffed

+ /usr/bin/gzip -dc /home/tkloczko/rpmbuild/SOURCES/python-myst-nb-0.17.2.tar.gz
+ /usr/bin/tar -xof -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd MyST-NB-0.17.2
+ /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ /usr/bin/cat /home/tkloczko/rpmbuild/SOURCES/python-myst-nb-UPGRADE-myst-parser-1.0.patch
+ /usr/bin/patch -p1 -s --fuzz=0 --no-backup-if-mismatch -f
1 out of 2 hunks FAILED -- saving rejects to file myst_nb/core/read.py.rej
1 out of 6 hunks FAILED -- saving rejects to file myst_nb/core/render.py.rej
1 out of 4 hunks FAILED -- saving rejects to file myst_nb/docutils_.py.rej
2 out of 3 hunks FAILED -- saving rejects to file myst_nb/sphinx_.py.rej
2 out of 2 hunks FAILED -- saving rejects to file pyproject.toml.rej

@mmcky
Copy link
Member

mmcky commented May 3, 2023

I need to take a closer look at how MystWarnings is working in create_warnings. I will need to loop back around to this PR in a couple days.

@mmcky
Copy link
Member

mmcky commented May 17, 2023

@chrisjsewell I have tried to mirror the new warnings_.py structure that has been put in place in myst-parser but I am getting mypy issues due to the mix of MystWarnings and MystNBWarnings. The render objects from myst-parser is returning MystWarnings when I need a union of both.

myst_nb/core/render.py:100: error: Argument "subtype" to "create_warning" of "DocutilsRenderer" has incompatible type "MystNBWarnings"; expected "MystWarnings"  [arg-type]

Would be interested in thoughts on how to resolve this if you've got time. Thanks.

@krassowski krassowski mentioned this pull request May 21, 2023
@krassowski
Copy link

In short term, would it be acceptable to just suppress these warnings with type: ignore?

@choldgraf
Copy link
Member

I'm personally fine with that - it feels like the pain people are feeling with a version capped and outdated MyST parser is stronger than the pain they might feel if some problem occurs because of this type mismatch.

I'd suggest we just try to get these changes in quickly and cut a release. Then keep an eye for new issues that might be related to this and provide guidance (inc telling people to version cap) if so.

@Yoshanuikabundi
Copy link
Contributor

I've fixed the type errors in a PR to this branch: aleivag#1

I did find a couple of changes to behavior from this process.

Just haven't heard back from @aleivag yet.

@choldgraf
Copy link
Member

Thanks for making that effort @Yoshanuikabundi - in that case I'd recommend the following course of action:

  • Give @aleivag a few days to review and merge @Yoshanuikabundi's PR.
  • If he doesn't respond in that time, I'll apply @Yoshanuikabundi's commit here (w/ attribution) and will push to @aleivag's branch.
  • We can then see if the test suite is happy.

@Yoshanuikabundi what were the behavior changes that you noticed?

@Yoshanuikabundi
Copy link
Contributor

what were the behavior changes that you noticed?

Oh there were a few warnings aleivag missed that I had to convert over to the new system, and I had to move some control flow around to handle the new warnings. I just meant that I had to do more than just tweak type annotations. So behaviour changes relative to this PR, not relative to the main branch.

@chrisjsewell
Copy link
Member

(FYI I'm not currently maintaining this repo: executablebooks/meta#961)

@choldgraf
Copy link
Member

choldgraf commented Jun 22, 2023

OK I've cherry-picked and pushed the two commits from @Yoshanuikabundi's PR to this branch. Let's see how that impacts tests and such.

EDIT: @Yoshanuikabundi it looks like there are a few errors in there:

/opt/hostedtoolcache/Python/3.8.17/x64/lib/python3.8/site-packages/myst_parser/parsers/docutils_.py:200: in attr_to_optparse_option
    at_options, default_str = _attr_to_optparse_option(attribute, default)
/opt/hostedtoolcache/Python/3.8.17/x64/lib/python3.8/site-packages/myst_parser/parsers/docutils_.py:185: in _attr_to_optparse_option
    raise AssertionError(
E   AssertionError: Configuration option read_as_md not set up for use in docutils.conf.

If you can get pytest passing on your branch then I can cherry-pick them here as well.

@Yoshanuikabundi
Copy link
Contributor

Yoshanuikabundi commented Jun 23, 2023

@choldgraf I was initially confused about this error because I don't see how anything I did could cause it (and also I don't understand it at all), so I checked out the commit on this branch before I did any work (a150582) and I get a very similar error locally there:

_____________________________ ERROR collecting tests/test_docutils.py ______________________________
tests/test_docutils.py:11: in <module>
    from myst_nb.docutils_ import Parser
myst_nb/docutils_.py:73: in <module>
    class Parser(MystParser):
myst_nb/docutils_.py:82: in Parser
    create_myst_settings_spec(NbParserConfig, "nb_"),
.soap/test/lib/python3.8/site-packages/myst_parser/parsers/docutils_.py:211: in create_myst_settings_spec
    return tuple(
.soap/test/lib/python3.8/site-packages/myst_parser/parsers/docutils_.py:212: in <genexpr>
    attr_to_optparse_option(at, getattr(defaults, at.name), prefix)
.soap/test/lib/python3.8/site-packages/myst_parser/parsers/docutils_.py:200: in attr_to_optparse_option
    at_options, default_str = _attr_to_optparse_option(attribute, default)
.soap/test/lib/python3.8/site-packages/myst_parser/parsers/docutils_.py:185: in _attr_to_optparse_option
    raise AssertionError(
E   AssertionError: Configuration option execution_mode not set up for use in docutils.conf.

So I think this might be from a change in a dependency? I'm looking into it but I'm in unfamiliar territory.

EDIT: Looks like it's both; I've created an environment with all the dependencies from the last time tests passed on this PR and that environment passes tests on a150582 but not 8f7f59e.

Conda environment from the last time tests passed
channels:
  - conda-forge
dependencies:
  - python=3.8
  - pip
  - pip:
    - Jinja2==3.1.2
    - MarkupSafe==2.1.2
    - Pygments==2.15.1
    - alabaster==0.7.13
    - babel==2.12.1
    - certifi==2023.5.7
    - charset-normalizer==3.1.0
    - docutils==0.19
    - idna==3.4
    - imagesize==1.4.1
    - importlib-metadata==6.6.0
    - packaging==23.1
    - pytz==2023.3
    - requests==2.30.0
    - snowballstemmer==2.2.0
    - sphinx==5.3.0
    - sphinxcontrib-applehelp==1.0.4
    - sphinxcontrib-devhelp==1.0.2
    - sphinxcontrib-htmlhelp==2.0.1
    - sphinxcontrib-jsmath==1.0.1
    - sphinxcontrib-qthelp==1.0.3
    - sphinxcontrib-serializinghtml==1.1.5
    - urllib3==2.0.2
    - zipp==3.15.0
    - GitPython==3.1.31
    - anyio==3.6.2
    - argon2-cffi==21.3.0
    - argon2-cffi-bindings==21.2.0
    - arrow==1.2.3
    - asttokens==2.2.1
    - attrs==23.1.0
    - backcall==0.2.0
    - beautifulsoup4==4.12.2
    - bleach==6.0.0
    - cffi==1.15.1
    - click==8.1.3
    - colorama==0.4.6
    - coverage==7.2.5
    - cycler==0.11.0
    - decorator==5.1.1
    - defusedxml==0.7.1
    - exceptiongroup==1.1.1
    - executing==1.2.0
    - fastjsonschema==2.16.3
    - fonttools==4.39.4
    - fqdn==1.5.1
    - gitdb==4.0.10
    - greenlet==2.0.2
    - importlib-resources==5.12.0
    - iniconfig==2.0.0
    - ipykernel==5.5.6
    - ipython==8.4.0
    - ipython-genutils==0.2.0
    - ipywidgets==8.0.6
    - isoduration==20.11.0
    - jedi==0.18.2
    - jsonpointer==2.3
    - jsonschema==4.17.3
    - jupyter-cache==0.6.1
    - jupyter-client==8.2.0
    - jupyter-core==5.3.0
    - jupyter-events==0.6.3
    - jupyter-server==2.5.0
    - jupyter-server-mathjax==0.2.6
    - jupyter-server-terminals==0.4.4
    - jupyterlab-pygments==0.2.2
    - jupyterlab-widgets==3.0.7
    - jupytext==1.14.5
    - kiwisolver==1.4.4
    - markdown-it-py==2.2.0
    - matplotlib==3.5.3
    - matplotlib-inline==0.1.6
    - mdit-py-plugins==0.3.5
    - mdurl==0.1.2
    - mistune==2.0.5
    - mpmath==1.3.0
    - myst-parser==1.0.0
    - nbclient==0.7.4
    - nbconvert==7.4.0
    - nbdime==3.2.1
    - nbformat==5.8.0
    - numpy==1.24.3
    - pandas==2.0.1
    - pandocfilters==1.5.0
    - parso==0.8.3
    - pexpect==4.8.0
    - pickleshare==0.7.5
    - pillow==9.5.0
    - pkgutil-resolve-name==1.3.10
    - platformdirs==3.5.1
    - pluggy==1.0.0
    - prometheus-client==0.16.0
    - prompt-toolkit==3.0.38
    - ptyprocess==0.7.0
    - pure-eval==0.2.2
    - pycparser==2.21
    - pyparsing==3.0.9
    - pyrsistent==0.19.3
    - pytest==7.3.1
    - pytest-cov==4.0.0
    - pytest-datadir==1.4.1
    - pytest-param-files==0.3.5
    - pytest-regressions==2.4.2
    - python-dateutil==2.8.2
    - python-json-logger==2.0.7
    - pyyaml==6.0
    - pyzmq==25.0.2
    - rfc3339-validator==0.1.4
    - rfc3986-validator==0.1.1
    - send2trash==1.8.2
    - six==1.16.0
    - smmap==5.0.0
    - sniffio==1.3.0
    - soupsieve==2.4.1
    - sqlalchemy==2.0.13
    - stack-data==0.6.2
    - sympy==1.12
    - tabulate==0.9.0
    - terminado==0.17.1
    - tinycss2==1.2.1
    - toml==0.10.2
    - tomli==2.0.1
    - tornado==6.3.2
    - traitlets==5.9.0
    - typing-extensions==4.5.0
    - tzdata==2023.3
    - uri-template==1.2.0
    - wcwidth==0.2.6
    - webcolors==1.13
    - webencodings==0.5.1
    - websocket-client==1.5.1
    - widgetsnbextension==4.0.7
    - -e .
```

Also MyST-parser 2.0 is out :upside_down_face:

@choldgraf
Copy link
Member

I suggest we roll back or pin as many non-myst-parser versions as we can until we get the tests passing, and then merge and iterate selectively from there.

This seems to be where the field that generates this error is coming from:

read_as_md: bool = dc.field(
default=False,
metadata={
"validator": instance_of(bool),
"help": "Read as the MyST Markdown format",
"sphinx_exclude": True,
},
repr=False,
)

cc also @agoose77 in case any of this is familiar? I think he's looked through the myst-nb codebase before.

agoose77 and others added 19 commits June 23, 2023 10:26
* docs: fix use of sphinx-design

* Update pyproject.toml
* Add ipywidgets javascript

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Angus Hollands <[email protected]>
* fix: use jsdelivr CDN

* chore: bump ipywidgets version to 8.0

* docs: add note about ipywidgets
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ebooks#504)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#505)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ablebooks#509)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@agoose77
Copy link
Collaborator

To be clear, the tests are failing because we broke something by moving to stringized annotations. I've reverted that part of the PR, which should make the tests pass 🤞

@LecrisUT
Copy link
Contributor

For sure that sounds like a functionality issue that needs to be addressed. I was thinking about having the tests be a bit more separated so it is easier to read. At some point the tests did not start to run because it detected bad typing errors, which does not tell us if the tests would actually fail for the user or not. Having those checks are great to have and address, but have them a bit separate so its easier to judge which one to prioritize and which ones can/should be resolved by dropping older version support

LecrisUT pushed a commit to LecrisUT/MyST-NB that referenced this pull request Jun 23, 2023
@choldgraf
Copy link
Member

Amazing! Thanks for this iteration folks. It looks like the only "failing" thing is the documentation having a few broken cross-reference links because the myst docs have changed. I would be fine merging this as-is in order to get it in. But if anyone wants to quickly fix those I'm happy to review and merge.

@Yoshanuikabundi
Copy link
Contributor

Yoshanuikabundi commented Jun 24, 2023

I've pushed fixes for the Intersphinx links that are breaking RTD to my branch: https://github.com/Yoshanuikabundi/MyST-NB/commits/upgrade-to-1

The docs build locally for me without warnings and I've spot checked a few of the links to check they still go to the same places. Was just a matter of updating to the new syntax for specifying Intersphinx links and fixing some ambiguities.

Thanks for taking a look @agoose77, I don't know that I ever would have figured that out!

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

OK I've pushed the latest patch from @Yoshanuikabundi and now all of the tests are happy - the codecov isn't but I think that's OK because this isn't adding functionality, just refactoring and updating because of upstream changes.

Thanks so much everybody for helping out here!

@choldgraf choldgraf merged commit 48c45c6 into executablebooks:master Jun 30, 2023
@welcome
Copy link

welcome bot commented Jun 30, 2023

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

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.