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

Add Flask 2.x compatibility + fix line number detection under python <= 3.8 and python 3.11 #72

Merged
merged 5 commits into from
May 21, 2023

Conversation

ValentinFrancois
Copy link

@ValentinFrancois ValentinFrancois commented Feb 17, 2023

Issue 1

Cause

When installing Flask 2.0 with python > 3.6, the chosen version of its Jinja2 dependency is >= 3.1.0:

$ docker run -it --rm python:3.7 bash
root@44ff1cecc27d:/# pip3 install Flask==2.0
Collecting Flask==2.0
  Downloading Flask-2.0.0-py3-none-any.whl (93 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 93.2/93.2 KB 2.5 MB/s eta 0:00:00
Collecting Jinja2>=3.0
  Downloading Jinja2-3.1.2-py3-none-any.whl (133 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 133.1/133.1 KB 7.5 MB/s eta 0:00:00

With python 3.6, the installed Jinja2 dependency seems to be < 3.1.0 which should not be problematic as I'll explain next.

$ docker run -it --rm python:3.6 bash
root@7a7dccf967f2:/# pip3 install Flask==2.0
Collecting Flask==2.0
  Downloading Flask-2.0.0-py3-none-any.whl (93 kB)
     |████████████████████████████████| 93 kB 1.6 MB/s 
Collecting Jinja2>=3.0
  Downloading Jinja2-3.0.3-py3-none-any.whl (133 kB)
     |████████████████████████████████| 133 kB 6.4 MB/s 

Consequences

import flask_selfdoc will then raise an ImportError because it tries to import the follow function and class that were deprecated in Jinja2 3.0 and removed in Jinja2 3.1:

Issue 2

Cause

The way that the line number of a decorator is detected changed across python versions:

  • python <= 3.8: stack()[1].lineno points to the line above the decorated function (= to the closest decorator, not necessarily the one that did the call to stack())
  • python 3.9 and 3.10: stack()[1].lineno points to the line of the decorated function
  • python 3.11: stack()[1].lineno points to the exact line of the decorator that did the call to stack()

Example:

    1   |def call_stack_and_get_lineno():
    2   |
    3   |    def decorator(func):
    4   |        calling_frame = stack()[1]
    5   |        print(calling_frame.lineno)
    6   |        return func
    7   |
    8   |    return decorator
    9   |
    10  |
    11  |@decorator1
    12  |@call_stack_and_get_lineno
    13  |@decorator2
    14  |def func():
    15  |    pass
  • python <= 3.8: will print line 13
  • python 3.9 and 3.10: will print line 14 (desired behaviour)
  • python 3.11: will print line 12

Consequences

  • unit tests don't pass for python 3.11
  • unit tests pass for python <= 3.8 but only because we adjust them (add a -1 offset to the detected line number)

What this PR does

Issue 1

  • Avoid Jinja2-related ImportError by trying the old and the new import for each Jinja function/class that is now deprecated
  • Remove the check Flask < 2.0 from the package dependencies
  • Adapt the ranges of tested python and Flask versions in the unit tests (WIP)
    • minimal-test.yml: remove python 3.7 (security support ends in 4 months), add python 3.10
    • test.yml: add Flask 2.1 (not compatible with python 3.6), add python 3.10 and 3.11 (not compatible with Flask 1.0)
  • Adapt the import of the Flask app context to version 2.3 where _app_ctx_stack is removed
  • Install target Flask version for tests using pip3 (easier than with poetry) and manually override its dependency versions when they're too recent (itsdangerous, Jinja2 and MarkupSafe for Flask 1.X)
  • List all installed packages before running tests

Issue 2

  • Fix line number detection by reading the source file and finding the next line starting with def

@jwg4
Copy link
Owner

jwg4 commented Feb 17, 2023

Thanks for this.

I am very in favour in principle; I want to make sure that we do this correctly and that we don't needlessly limit compatibility with older versions.

@ValentinFrancois
Copy link
Author

@jwg4 cool, I clarified the PR description a bit.

I think I spotted an issue in minimal-test.yml and test.yml that was overwriting the target Flask version from the matrix.

I also added python 3.10 to see which is the oldest supported version of Flask (I already excluded 1.0 based on your README.md).

@jwg4
Copy link
Owner

jwg4 commented Feb 17, 2023

  1. looks good in principle
  2. thank you for sorting out the jinja2 mess which I was dreading!
  3. removing Python 3.7 from the quick test matrix seems fine.
  4. adding flask and python versions to the main test matrix and specifying not to run certain toxic combinations also seems like the right decision.
  5. I wonder if we should add 'latest' to the Python version specs, any thoughts?
  6. as you can see above, some of the combinations fail - it seems that poetry will not allow you to specify a version which has a more restrictive Python version than what you have set for your own project, even if your Python version matches both specs. This seems right in principle, but I wonder if there's a way of overriding it for these purposes?
  7. the fact that 6 has only now come up suggests that you are right about the flask versions not actually being correct before your fix.
    I think this should be merged as soon as we can find a solution or a workaround for 6.

@jwg4
Copy link
Owner

jwg4 commented Feb 19, 2023

thank you so much for this, I have left several comments. let me know if any that don't make sense or you don't think belong to this change.

as you will see, some of them relate to the failing tests which include several causes

runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v2
- uses: actions/setup-python@v2
- uses: actions/checkout@v3
Copy link
Author

Choose a reason for hiding this comment

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

GitHub was encouraging us to update to the most recent action versions, seems harmless

- name: Force Python version (linux/macOS)
if: matrix.os != 'windows-latest'
# sed command for macOS: https://stackoverflow.com/a/44864004
run: sed -i.bak 's/python = "^3.6"/python = "~${{ matrix.python-version }}"/' pyproject.toml
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't find a better way than sed-replacing the python version in pyproject.toml to the fixed, desired 3.X version.

- name: Install requirements
run: poetry install

- name: List installed package versions for manual inspection
run: poetry --version && poetry show
Copy link
Author

Choose a reason for hiding this comment

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

just to control what packages we exactly installed in the end

run: poetry add Flask==${{ matrix.flask-version }}

- name: Overwrite Flask dependencies for legacy install
if: matrix.flask-version < '2.0' && matrix.flask-version != 'latest'
Copy link
Author

Choose a reason for hiding this comment

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

some packages that are now too recent when installed via the Flask 1.X dependencies

- name: Run doctests
run: poetry run doctest

check_pip_install:
Copy link
Author

Choose a reason for hiding this comment

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

add a different test strategy:

  • given a specific Python runtime version
  • given a specific Flask version installed
  • try installing flask-selfdoc via pip and check that we can use it without ImportError
    => this basically tests the dependency configuration in pyproject.toml


- name: Install current flask-selfdoc from GitHub
run: |
git_url=$(echo "${{ github.repositoryUrl }}@${{ github.head_ref }}" | sed "s/git:/git+https:/")
Copy link
Author

Choose a reason for hiding this comment

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

nice way to install from the latest commit of the current PR branch

run: pip3 install werkzeug==2.0.3

- name: List installed package versions for manual inspection
run: python3 --version && pip3 list
Copy link
Author

Choose a reason for hiding this comment

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

control what package versions we exactly installed

- name: List installed package versions for manual inspection
run: python3 --version && pip3 list

- name: Check that Flask version did not change
Copy link
Author

Choose a reason for hiding this comment

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

control that pip's dependency solver didn't need to update the already installed version of Flask

@@ -7,7 +7,11 @@ license = "MIT"

[tool.poetry.dependencies]
python = "^3.6"
Flask = "^1.0"
Flask = [
Copy link
Author

Choose a reason for hiding this comment

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

express Flask dependency conditionally (depending on the installed version of Python)

Copy link
Author

Choose a reason for hiding this comment

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

I think this solves point 6 that you mentioned here: #72 (comment)

However it is still not possible to rely on the own dependencies config of Flask 1.X because it accepts too recent versions of itsdangerous, Jinja2 and MarkupSafe that cause ImportError etc.
So these have to be manually overwritten with older, compatible versions like I did in test.yml

@ValentinFrancois
Copy link
Author

@jwg4 sorry I didn't see your previous comments while I was still struggling to solve the issues with poetry & dependency handling.
I think I've come to a good solution, I left a few comments explaining my recent changes.

The only thing I'm not sure about is why the output of flask_selfdoc gets different line numbers depending on the installed python/Flask versions. For now I added some pre-processing function to remove the line numbers from the compared outputs in test_check_example.py but maybe you'll want to investigate this deeper.

@ValentinFrancois
Copy link
Author

I tried adding tests for latest python as you suggested (3.x, currently evaluating to 3.11.2) but under this version, the unit tests for line numbers don't pass:

image

So I think there's something defninitely weird with inspect and how it calculates line numbers of caller frames.
Even with a specific python version, the tests can fail depending on the OS because of different line numbers:

image

I'm removing the code I had added to ignore these line number differences, because it seems to be more important than I first thought.
By the way, I rewrote test.yml to install the target Flask version with pip3, which in the end was easier to use (especially under windows).

@jwg4
Copy link
Owner

jwg4 commented Feb 20, 2023

My understanding is that the code which retrieves the line number where a function is defined has changed to take decorators into account in a different way. I believe this is a change in a python built-in which flask uses and does not have control over. I don't know how OS type affects this.

@ValentinFrancois
Copy link
Author

Interesting, now python 3.11 seems to have changed again the way line number is calculated.

In this unit test, the frame now points to this line:
image

I suppose this is related to this: https://docs.python.org/3/whatsnew/3.11.html#whatsnew311-inspect but they didn't detail this behavior.

Anyway I figured out we could actually just read the source file and offset the line number by 1 as long as we don't find a line starting with def . Do you think it's a good idea? I suppose it won't work for cythonized code but it's probably ok since this package is meant to document source files, and not files optimized for production?


try:
from flask import _app_ctx_stack as stack
Copy link
Author

Choose a reason for hiding this comment

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

With python 3.11 and Flask 2.2.3 I was getting _app_ctx_stack' is deprecated and will be removed in Flask 2.3.
See pallets/flask#4682

@@ -153,7 +254,7 @@ def generate(self, groups='all', sort=None):
methods=sorted(list(rule.methods)),
rule="%s" % rule,
endpoint=rule.endpoint,
docstring=func.__doc__,
docstring=func.__doc__.strip(' ') if func.__doc__ else None,
Copy link
Author

Choose a reason for hiding this comment

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

I thought this could be more practical for teams where some developers have an IDE that automatically strips the trailing whitespaces

def json(self,
groups='all',
indent: Optional[int] = None,
separators: Optional[Tuple] = (',', ':')):
Copy link
Author

Choose a reason for hiding this comment

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

I'm adding some options that are passed to json.dumps, I found it practical to debug the JSON output comparisons in the unit tests. Default options produce the same output as jsonify (i.e. minified JSON)

# The old version chooses the location of the first decorator,
# the new version chooses the location of the 'def' keyword.
# We detect the version and support both.
NEW_FN_OFFSETS = sys.version_info >= (3, 8)
Copy link
Author

Choose a reason for hiding this comment

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

we wouldn't need this anymore because I now try to add some offset if stack()[1].lineno doesn't point to a line starting by def

@@ -1 +1,126 @@
{"endpoints":[{"args":[],"docstring":"Return all posts.","methods":["GET","HEAD","OPTIONS"],"rule":"/"},{"args":[],"docstring":"Admin interface.","methods":["GET","HEAD","OPTIONS"],"rule":"/admin"},{"args":[["greeting","Hello"],["id",null]],"docstring":"Return the user for the given id.","methods":["GET","HEAD","OPTIONS"],"rule":"/greet/<greeting>/user/<int:id>"},{"args":[["id",-1]],"docstring":"Return the user for the given id.","methods":["GET","HEAD","OPTIONS"],"rule":"/hello/user/<int:id>"},{"args":[],"docstring":"Create a new post.\n Form Data: title, content, authorid.\n ","methods":["OPTIONS","POST"],"rule":"/post"},{"args":[["id",null]],"docstring":"Return the post for the given id.","methods":["GET","HEAD","OPTIONS"],"rule":"/post/<int:id>"},{"args":[],"docstring":"Return all posts.","methods":["GET","HEAD","OPTIONS"],"rule":"/posts"},{"args":[["id",null]],"docstring":"Return the user for the given id.","methods":["GET","HEAD","OPTIONS"],"rule":"/user/<int:id>"},{"args":[],"docstring":"Return all users.","methods":["GET","HEAD","OPTIONS"],"rule":"/users"},{"args":[],"docstring":"Creates a new user.\n Form Data: username.\n ","methods":["OPTIONS","POST"],"rule":"/users"}]}
{
Copy link
Author

Choose a reason for hiding this comment

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

this was to debug the unit tests, we can re-minify the output if you prefer

"rule": "/users"
}
]
{
Copy link
Author

Choose a reason for hiding this comment

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

here I just replaced tabs by 2 spaces and removed the trailing whitespaces (more common format imo)

@@ -1 +1,185 @@
[{"args":["None"],"defaults":{},"docstring":"Return all posts.","endpoint":"get_posts","location":{"filename":"%PATH%/examples/simple/blog.py","line":45},"methods":["GET","HEAD","OPTIONS"],"rule":"/"},{"args":["None"],"defaults":{},"docstring":"Admin interface.","endpoint":"admin","location":{"filename":"%PATH%/examples/simple/blog.py","line":113},"methods":["GET","HEAD","OPTIONS"],"rule":"/admin"},{"args":["greeting","id"],"defaults":{"greeting":"Hello"},"docstring":"Return the user for the given id.","endpoint":"greet_user","location":{"filename":"%PATH%/examples/simple/blog.py","line":96},"methods":["GET","HEAD","OPTIONS"],"rule":"/greet/<greeting>/user/<int:id>"},{"args":["id"],"defaults":{"id":-1},"docstring":"Return the user for the given id.","endpoint":"hello_to_user","location":{"filename":"%PATH%/examples/simple/blog.py","line":86},"methods":["GET","HEAD","OPTIONS"],"rule":"/hello/user/<int:id>"},{"args":["None"],"defaults":{},"docstring":"Create a new post.\n Form Data: title, content, authorid.\n ","endpoint":"post_post","location":{"filename":"%PATH%/examples/simple/blog.py","line":59},"methods":["OPTIONS","POST"],"rule":"/post"},{"args":["id"],"defaults":{},"docstring":"Return the post for the given id.","endpoint":"get_post","location":{"filename":"%PATH%/examples/simple/blog.py","line":52},"methods":["GET","HEAD","OPTIONS"],"rule":"/post/<int:id>"},{"args":["None"],"defaults":{},"docstring":"Return all posts.","endpoint":"get_posts","location":{"filename":"%PATH%/examples/simple/blog.py","line":45},"methods":["GET","HEAD","OPTIONS"],"rule":"/posts"},{"args":["id"],"defaults":{},"docstring":"Return the user for the given id.","endpoint":"get_user","location":{"filename":"%PATH%/examples/simple/blog.py","line":79},"methods":["GET","HEAD","OPTIONS"],"rule":"/user/<int:id>"},{"args":["None"],"defaults":{},"docstring":"Return all users.","endpoint":"get_users","location":{"filename":"%PATH%/examples/simple/blog.py","line":72},"methods":["GET","HEAD","OPTIONS"],"rule":"/users"},{"args":["None"],"defaults":{},"docstring":"Creates a new user.\n Form Data: username.\n ","endpoint":"post_user","location":{"filename":"%PATH%/examples/simple/blog.py","line":103},"methods":["OPTIONS","POST"],"rule":"/users"}]
[
Copy link
Author

Choose a reason for hiding this comment

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

same as above, it was easier to debug like this but we can re-minify the output

return response


def get_decorator_frame_info(frame) -> dict:
Copy link
Author

Choose a reason for hiding this comment

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

this is the fix for the line numbers that should work with any python version >= 3.6, with some documentation

@@ -44,7 +145,10 @@ def init_app(self, app):
self.add_custom_template_filters(app)

def teardown(self, exception):
ctx = stack.top # noqa: F841
if _cv_app is not None:
Copy link
Author

Choose a reason for hiding this comment

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

I suppose this is some hack to avoid destroying the context or something like that?
I tried to do the same with the new app context object from Flask 2.3.

@ValentinFrancois ValentinFrancois changed the title Add Flask 2.x compatibility Add Flask 2.x compatibility + fix line number detection under python <= 3.8 and python 3.11 Feb 21, 2023
@ValentinFrancois
Copy link
Author

In the end the PR got bigger than I first planned, but all tests are green now! I updated the PR description to my latest changes

@ValentinFrancois
Copy link
Author

just made it work again with pytest, I suppose it didn't like the double inheritance trick

@ValentinFrancois
Copy link
Author

Sorry I didn't realize that
"${{ github.repositoryUrl }}@${{ github.head_ref }}" would not work on your side,
because ${{ github.repositoryUrl }} points to the repository where the action is running, and not the repository where the PR comes from.

I switched to "${{ github.event.pull_request.head.repo.git_url }}@${{ github.event.pull_request.head.ref }}",
based on https://stackoverflow.com/a/65027014.

Now the check_pip_install tests should always install from the branch in the source repo (forked or not), which should work on your side too.

@ValentinFrancois
Copy link
Author

My bad, I forgot to do the same in the install step for Windows. Hoping it's the last iteration

@ValentinFrancois
Copy link
Author

Anything still blocking the merge?

I see one job that seems to be stuck for some reason
image

@ValentinFrancois
Copy link
Author

ping

@jwg4
Copy link
Owner

jwg4 commented Mar 18, 2023

hi, sorry for delay, I've been very busy with some other work.

@ValentinFrancois
Copy link
Author

No worries, just let me know if there's anything else I can help with.
I don't really understand why a single job has stayed stuck this whole time:
image

@jwg4
Copy link
Owner

jwg4 commented Apr 3, 2023

do you think that you could rebase and squash your changes?

either into a single commit, into a small number of commits (if it's easy to separate the different changes like that), or failing that, just to remove the changes which were later rolled back or made obsolete by other changes.

after that's done I will give a final review and merge quickly.

@jwaxy
Copy link

jwaxy commented May 5, 2023

is there any progress in merging this?

@ValentinFrancois
Copy link
Author

Oh thanks for the reminder! Just rebased into a few commits.

@jwaxy
Copy link

jwaxy commented May 6, 2023

And btw is this library compatible with flask-restful?

@ValentinFrancois
Copy link
Author

No, the auto documentation logic relies on the Flask.route decorator which is not used in flask-restful.

Copy link
Owner

@jwg4 jwg4 left a comment

Choose a reason for hiding this comment

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

looks good

run: poetry add flask==${{ matrix.flask-version }}
run: pip3 install poetry
- name: Force the Flask & python versions
run: poetry add Flask==${{ matrix.flask-version }} --python ${{ matrix.python-version }}
Copy link
Owner

Choose a reason for hiding this comment

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

this looks good, thank you for finding how to solve this

- name: Install requirements
run: poetry install
- name: List installed package versions for manual inspection
run: poetry --version && poetry show
Copy link
Owner

Choose a reason for hiding this comment

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

👍

run: poetry add flask==${{ matrix.flask-version }}
run: pip3 install poetry
- name: Overwrite Flask dependencies for legacy install
if: matrix.flask-version < '2.0' && matrix.flask-version != 'latest'
Copy link
Owner

Choose a reason for hiding this comment

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

is this a typo? you are checking flask-version twice

Copy link
Author

Choose a reason for hiding this comment

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

(Sorry, just seeing this now)

I wasn't sure how matrix.flask-version < 2.0 would behave in the case where matrix.flask-version == 'latest'.
Now I think matrix.flask-version < '2.0' would be sufficient.

- name: Overwrite Flask dependencies for legacy install
if: matrix.flask-version < '2.0' && matrix.flask-version != 'latest'
run: |
poetry add Jinja2=^2.0 --python ${{ matrix.python-version }}
Copy link
Owner

Choose a reason for hiding this comment

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

this is making me think that we have to configure these Flask + deps correctly in pyproject.toml. otherwise the tests will run correctly but people who install it themselves will come up against the jinja2 problems.

- name: Run unit tests
run: poetry run test
- name: Run doctests
run: poetry run doctest

check_pip_install:
Copy link
Owner

Choose a reason for hiding this comment

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

Let's put this in a different workflow file?

os: [ubuntu-18.04, macos-latest]
exclude:
# starting from Flask 2.1.0, python 3.6 is no longer supported:
- flask-version: '2.1'
Copy link
Owner

Choose a reason for hiding this comment

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

perhaps we should just allow all runs with python 3.6 to fail (or skip them), since it's EOL?

Copy link
Author

Choose a reason for hiding this comment

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

I guess now that python 3.6 is EOL we can indeed simplify the logic and stop testing on it

- name: Install current flask-selfdoc from GitHub
run: |
git_url=$(echo "${{ github.repositoryUrl }}@${{ github.head_ref }}" | sed "s/git:/git+https:/")
pip3 install ${git_url}
Copy link
Owner

Choose a reason for hiding this comment

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

this is currently failing

exit 1
fi
- name: Try importing flask_selfdoc
run: python3 -c "import flask_selfdoc"
Copy link
Owner

Choose a reason for hiding this comment

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

arguably we could run some tests or an example after importing this way?

Copy link
Author

Choose a reason for hiding this comment

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

If I remember correctly the issue I fixed happened only at import time, but yes running an example is a good idea.

return data.replace(file_path, "%PATH%")
return data.replace(file_path, "%PATH%")

def remove_line_info(self, data):
Copy link
Owner

Choose a reason for hiding this comment

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

this is currently failing, I think it the cases where location already isn't a dict?

self.assertEqual(data, expected)
data = self.remove_line_info(data)
try:
self.assertEqual(data, expected)
Copy link
Owner

Choose a reason for hiding this comment

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

I believe you can pass in a 3rd argument to assertEqual which is an error message. if we do that here, with the error message containing the data and the expected data, we can avoid the try/except

@jwg4 jwg4 merged commit d594157 into jwg4:main May 21, 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.

3 participants