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

Workaround lru_cache decorator on method garbage collection problem #344

Merged
merged 7 commits into from
May 6, 2022

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Apr 22, 2022

These changes close #330

@lru_cache, if used on a method, retains the class instance beyond the point it would normally be garbage collected. This workaround is from https://stackoverflow.com/a/68550238/3217306

This PR also includes some minor GH Actions test workflow updates and removes pytest-asyncio (see pytest-dev/pytest-asyncio#257 (comment))

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Does not need tests
  • No change log entry required (invisible to users)
  • No documentation update required.
  • No core dependency changes.

lru_cache, if used on a method, retains the class instance beyond the point it would normally be garbage collected
@MetRonnie MetRonnie added this to the 1.0.2 milestone Apr 22, 2022
@MetRonnie MetRonnie self-assigned this Apr 22, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2022

Codecov Report

Merging #344 (0617e9c) into master (dcb91cc) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   77.76%   78.05%   +0.28%     
==========================================
  Files          10       10              
  Lines        1048     1048              
  Branches      202      202              
==========================================
+ Hits          815      818       +3     
+ Misses        196      194       -2     
+ Partials       37       36       -1     
Impacted Files Coverage Δ
cylc/uiserver/authorise.py 89.56% <100.00%> (+0.86%) ⬆️
cylc/uiserver/workflows_mgr.py 82.94% <100.00%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcb91cc...0617e9c. Read the comment docs.

@MetRonnie MetRonnie marked this pull request as draft April 25, 2022 09:13
@MetRonnie
Copy link
Member Author

MacOS test failure is pretty strange, doesn't seem related to this change, but doesn't fail on master though

@MetRonnie MetRonnie marked this pull request as ready for review April 25, 2022 17:16
@MetRonnie MetRonnie added the infrastructure GH Actions etc. label Apr 25, 2022
@MetRonnie
Copy link
Member Author

MetRonnie commented Apr 25, 2022

Well I don't know why it's resolved itself. Possibly needed asyncio_mode=strict in pytest.ini

However, still getting these printed out after the end of the pytest output

Task was destroyed but it is pending!
task: <Task pending name='Task-57' coro=<WorkflowsManager.run() running at
    /home/runner/work/cylc-uiserver/cylc-uiserver/cylc/uiserver/workflows_mgr.py:435>
    wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7f79f6f8ce50>()]>
    cb=[IOLoop.add_future.<locals>.<lambda>() at
    /opt/hostedtoolcache/Python/3.9.12/x64/lib/python3.9/site-packages/tornado/ioloop.py:688]>

@datamel
Copy link
Contributor

datamel commented Apr 26, 2022

Well I don't know why it's resolved itself. Possibly needed asyncio_mode=strict in pytest.ini

However, still getting these printed out after the end of the pytest output

Task was destroyed but it is pending!
task: <Task pending name='Task-57' coro=<WorkflowsManager.run() running at
    /home/runner/work/cylc-uiserver/cylc-uiserver/cylc/uiserver/workflows_mgr.py:435>
    wait_for=<Future pending cb=[<TaskWakeupMethWrapper object at 0x7f79f6f8ce50>()]>
    cb=[IOLoop.add_future.<locals>.<lambda>() at
    /opt/hostedtoolcache/Python/3.9.12/x64/lib/python3.9/site-packages/tornado/ioloop.py:688]>

I have raised this with @oliver-sanders before, from memory, it only occurs when the python version is >=3.9. See conversation after #324 (review)

@MetRonnie
Copy link
Member Author

It's happening for Python 3.7 on this branch and master: https://github.com/cylc/cylc-uiserver/runs/6160205704?check_suite_focus=true#step:7:80

I saw this comment on #324:

The warnings are due to a regression in jupyter_server:

https://github.com/jupyter-server/jupyter_server/pull/588/files#diff-3cd116aa8a4bea11f638c9209aa3b58e13a3a0aad127d310b0198b4931ca962cR305-R306

Is it worth raising an issue there?

@datamel
Copy link
Contributor

datamel commented Apr 26, 2022

Is it worth raising an issue there?

Looks like they are aware: jupyter-server/jupyter_server#672 (comment)

pyproject.toml Outdated
@@ -22,3 +22,4 @@ testpaths = [
markers = [
'integration: tests which run servers and try to connect to them'
]
asyncio_mode = 'strict' # 'auto' seems to conflict with pytest-tornasync
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, this async means tests need an explicit pytest.mark.asyncio? Need to check that relevant tests are decorated correctly otherwise they will be skipped from here on.

@MetRonnie MetRonnie marked this pull request as draft April 27, 2022 11:21
@MetRonnie
Copy link
Member Author

Hmm, adding @pytest.mark.asyncio to the async tests that were missing them brings back this error:

FAILED cylc/uiserver/tests/test_auth.py::test_cylc_handler -
    tornado.simple_httpclient.HTTPTimeoutError: Timeout during request

Seems to be only the tests that use the jp_fetch fixture

Seems to conflict with pytest-tornasync.
And pytest-tornasync should be capable of running the async tests itself anyway
@MetRonnie MetRonnie marked this pull request as ready for review April 27, 2022 16:43
@MetRonnie
Copy link
Member Author

After a brief look at pytest-tornasync it seems the plugin has a similar behaviour to pytest-asyncio with async_mode=auto: It tries to run all tests that are coroutines in a tornado.IoLoop. When both plugins try to do the same thing, unexpected things happen.

pytest-dev/pytest-asyncio#257 (comment)

@oliver-sanders
Copy link
Member

When both plugins try to do the same thing, unexpected things happen.

Makes sense, thanks for investigating this.

Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

I have manually tested this, working well. This was the initial implementation, @oliver-sanders just checking there is not a reason for the requested change, I am struggling to remember... be7d04d#r728858282

@MetRonnie
Copy link
Member Author

#204 (comment) - that was before we realised about the problem of using @lru_cache to decorate methods: https://stackoverflow.com/q/33672412/3217306

@MetRonnie MetRonnie changed the title Workaround lru_cache garbage collection problem Workaround lru_cache decorator on method garbage collection problem Apr 29, 2022
@datamel
Copy link
Contributor

datamel commented Apr 29, 2022

#204 (comment) - that was before we realised about the problem of using @lru_cache to decorate methods: https://stackoverflow.com/q/33672412/3217306

Yes, I understand why it is being changed now, I am wondering if there was a specific reason why it was requested to be changed before. As in, was there a problem with that implementation that I can not think of now?

@MetRonnie
Copy link
Member Author

It's a little yukky compared to the cleanliness of simply decorating the method

@datamel
Copy link
Contributor

datamel commented May 5, 2022

This branch has conflicts now. If you want to get this rebased I can give it a final check before it goes in @MetRonnie?

@MetRonnie
Copy link
Member Author

Will merge master into this branch rather than rebase - makes the conflict resolution diff easy to see

@oliver-sanders oliver-sanders merged commit 0692266 into cylc:master May 6, 2022
@MetRonnie MetRonnie deleted the cache-bug branch May 6, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Authorization class caching
4 participants