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

Enforce Jinja2 3.x #1079

Closed
CasperWA opened this issue Feb 21, 2022 · 8 comments · Fixed by #1081 or #1087
Closed

Enforce Jinja2 3.x #1079

CasperWA opened this issue Feb 21, 2022 · 8 comments · Fixed by #1081 or #1087
Assignees
Labels
blocking For issues/PRs that are blocking other PRs. bug Something isn't working CI Continuous Integration - GitHub Actions issues (NOT related to the repository Action) dependency_updates Issues pertaining to updates to our dependencies that are breaking the eager build

Comments

@CasperWA
Copy link
Member

As seen in this workflow Jinja2 v3.x is uninstalled and Jinja2 v2.x is installed instead when installing the client requirements.

This results in an ImportError from markupsafe (see this issue).
There are two solutions to this issue:

  1. Pin markupsafe
  2. Enforce Jinja2 3.x

I prefer the second option as we're already using Jinja2 v3.x in the CI through requirements.txt, but allowing Jinja2 v2.x in setup.py. However, it may be that some of the client dependencies do not allow this. Then we'd need to go for the first option above I suppose. But would need to keep an eye out for when the dependency then updates to use Jinja2 v3.x and remove the markupsafe pin.

@CasperWA CasperWA added bug Something isn't working dependency_updates Issues pertaining to updates to our dependencies that are breaking the eager build CI Continuous Integration - GitHub Actions issues (NOT related to the repository Action) blocking For issues/PRs that are blocking other PRs. labels Feb 21, 2022
@ml-evs
Copy link
Member

ml-evs commented Feb 21, 2022

Our use of jinja is so superficial (no loops, only one conditional) that I wouldn't be against dropping the dep and just writing a static HTML file ourselves when the server starts. Only issue would be how careful we need to be with input sanitation, but we can use pydantic for that anyway.

@CasperWA
Copy link
Member Author

Fair enough. I think there's an issue then with the client dependencies, which may not have solved this problem - but that's essentially out of our control, I suppose.

@ml-evs
Copy link
Member

ml-evs commented Feb 23, 2022

Just revisiting this, isn't the whole problem that AiiDA is pinned to jinja2<=2 , which is why we had to allow it too? The tests only fail after the aiida install step

@ml-evs
Copy link
Member

ml-evs commented Feb 23, 2022

You can see us having the exact same discussion less than a year ago, hah #838

@CasperWA
Copy link
Member Author

Just revisiting this, isn't the whole problem that AiiDA is pinned to jinja2<=2 , which is why we had to allow it too? The tests only fail after the aiida install step

Yeah. Can we try to maybe install jinja2>=3 after installing AiiDA to see if it works?

@ml-evs
Copy link
Member

ml-evs commented Feb 28, 2022

@ml-evs
Copy link
Member

ml-evs commented Feb 28, 2022

Ugh, my bad, I didn't remove the old pinned version from the setup.py.

@CasperWA
Copy link
Member Author

CasperWA commented Feb 28, 2022

Ugh, my bad, I didn't remove the old pinned version from the setup.py.

Right! Well spotted.

Create a new PR that references and closes #1079 and then we can close #1086 if the CD succeeds afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking For issues/PRs that are blocking other PRs. bug Something isn't working CI Continuous Integration - GitHub Actions issues (NOT related to the repository Action) dependency_updates Issues pertaining to updates to our dependencies that are breaking the eager build
Projects
None yet
2 participants