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

Fixup: PR 811: install an upper-bounded version of Jinja2 during documentation build #812

Merged
merged 7 commits into from Jan 26, 2023
Merged

Fixup: PR 811: install an upper-bounded version of Jinja2 during documentation build #812

merged 7 commits into from Jan 26, 2023

Conversation

jayaddison
Copy link
Contributor

I'm hopeful that this will resolve the continuous integration failure in #811.

It looks like the ImportError: cannot import name 'environmentfilter' from 'jinja2' error seen in the build logs there is a symptom of using some less-recent versions of sphinx (as packaged with stable/LTS Ubuntu versions) with quite-recent versions of Jinja2 -- particularly v3.1.0 of Jinja2 and beyond.

Until a recent-enough version of sphinx is available in the ubuntu-latest GitHub runner image, it should be possible to install an upper-bounded version of Jinja2 as a workaround.

(please note: this pull request is opened against the master branch instead of the #811 pull request branch so that it is eligible for continuous integration -- but it's intended to be merged into #811)

ElDeveloper and others added 3 commits January 13, 2023 11:49
Set objects are no longer allowed as an index argument for a DataFrame
constructor.

Fixes #810
@ElDeveloper
Copy link
Member

@jayaddison thanks looks like this fixed the broken build. Would you mind moving the version restriction from the config file to the setup.py file? There's already a minimum jinja version specified there so it wouldn't be crazy to restrict to a specific version range. The better long term solution will be to update to a newer Sphinx version, but that might be complicated 🤷‍♂️ .

…dependency during sphinx documentation build"

This reverts commit de26d4d.
@jayaddison
Copy link
Contributor Author

Good idea, thanks @ElDeveloper - I hadn't noticed the setup.py requirements.

Although.. while starting work on this, I've noticed that jinja2 is also a dependency of the emperor tool itself (not only the documentation build).

I'm thinking about whether we could selectively apply that upper-bound only for the documentation build.

@jayaddison
Copy link
Contributor Author

Could you run continuous integration once more, @ElDeveloper?

(the approach taken in ae84d8e is maybe a little unusual, but valid I think - jinja2 is specified with an upper-bound in the doc dependencies, and without one in the base dependencies -- so that the restriction only applies in environments where documentation-build support is requested)

@ElDeveloper
Copy link
Member

Thanks @jayaddison, this makes a lot of sense. Not sure how setup.py will react to a repeated requirement for Jinja.

@jayaddison
Copy link
Contributor Author

It seemed to behave correctly based on some local testing (installing both all and doc dependencies -- the intended corresponding version range(s) were applied in each case). I agree it seems unclear though (and if it seems unclear, maybe it's not the best idea). Open to alternative suggestions.

@ElDeveloper ElDeveloper merged commit 6163b4e into biocore:master Jan 26, 2023
@ElDeveloper
Copy link
Member

@jayaddison I think this is a fine solution. Thanks so much!

@jayaddison
Copy link
Contributor Author

Thanks - you're welcome!

@jayaddison jayaddison deleted the pr-811-accompaniment/jinja2-environmentfilter-upperbound branch January 26, 2023 19:22
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

2 participants