biocore / emperor

Emperor a tool for the analysis and visualization of large microbial ecology datasets
http://biocore.github.io/emperor/
Other
52 stars 50 forks source link

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

Closed jayaddison closed 1 year ago

jayaddison commented 1 year ago

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 commented 1 year ago

@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 🤷‍♂️ .

jayaddison commented 1 year ago

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 commented 1 year ago

Could you run continuous integration once more, @ElDeveloper?

(the approach taken in ae84d8ea8a29758f28d20db45f08a5b394adeefc 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 commented 1 year ago

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

jayaddison commented 1 year ago

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 commented 1 year ago

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

jayaddison commented 1 year ago

Thanks - you're welcome!