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

Fix cannot uncache novel objs #4493

Merged
merged 9 commits into from Dec 15, 2021
Merged

Fix cannot uncache novel objs #4493

merged 9 commits into from Dec 15, 2021

Conversation

greenape
Copy link
Member

@greenape greenape commented Nov 8, 2021

Closes #4008

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

Deals with queries which were defined outside the server process (e.g. interactive session, defined in an imported module in an interactive session) being un-invalidatable outside the defining session by unpickling them to a stub class which does nothing but preserve their dependency links and query id.

I was hoping to deal with this by pickling them using dill, but that can't pickle them either outside of a __main__. One could also pickle to a stub where not pickleable, but I think this is marginally more robust because it doesn't rely on everyone immediately using an up to date FlowMachine.

@greenape greenape added bug Something isn't working FlowMachine Issues related to FlowMachine Needs Review PR that is ready for review by a human labels Nov 8, 2021
@cypress
Copy link

cypress bot commented Nov 8, 2021



Test summary

43 0 0 0Flakiness 0


Run details

Project FlowAuth
Status Passed
Commit cfc55e5
Started Dec 15, 2021 4:29 PM
Ended Dec 15, 2021 4:34 PM
Duration 05:01 💡
OS Linux Debian - 10.5
Browser Electron 94

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@codecov
Copy link

codecov bot commented Nov 26, 2021

Codecov Report

Merging #4493 (cfc55e5) into master (9352d86) will increase coverage by 0.00%.
The diff coverage is 92.30%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4493   +/-   ##
=======================================
  Coverage   93.43%   93.44%           
=======================================
  Files         273      273           
  Lines       10696    10711   +15     
  Branches     1282     1285    +3     
=======================================
+ Hits         9994    10009   +15     
  Misses        565      565           
  Partials      137      137           
Flag Coverage Δ
autoflow_unit_tests 93.03% <ø> (ø)
flowapi_unit_tests 71.83% <ø> (ø)
flowauth_unit_tests 78.41% <ø> (ø)
flowclient_unit_tests 74.31% <ø> (ø)
flowetl_unit_tests 95.49% <ø> (ø)
flowkit_jwt_generator_unit_tests 95.10% <ø> (ø)
flowmachine_unit_tests 91.29% <92.30%> (+0.01%) ⬆️
integration_tests 66.76% <11.53%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flowmachine/flowmachine/core/cache.py 93.69% <90.47%> (-0.37%) ⬇️
flowmachine/flowmachine/core/query.py 95.69% <100.00%> (+0.48%) ⬆️

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 9352d86...cfc55e5. Read the comment docs.


try:
return get_query_object_by_id(connection, query_id)
except (EOFError, ModuleNotFoundError, AttributeError) as exc:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want UnpicklingError here as well?

Co-Authored-By: Thingus <3050091+Thingus@users.noreply.github.com>
q.store(store_dependencies=True).result()
Query._QueryPool.clear()
yield q_id, nested_id

Copy link
Contributor

Choose a reason for hiding this comment

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

Only extra test I can think of here is nesting a novel in a not-novel or vice versa

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, novel in not-novel would never happen (if I've understood correctly) and from what I've seen, nesting two novel queries encapsulates nesting a not-novel in a novel, so disregard.

Copy link
Member

@jc-harrison jc-harrison left a comment

Choose a reason for hiding this comment

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

👍

@greenape greenape added the ready-to-merge Label indicating a PR is OK to automerge label Dec 15, 2021
@mergify mergify bot merged commit 610019a into master Dec 15, 2021
@mergify mergify bot deleted the fix-cannot-uncache-new-objs branch December 15, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working FlowMachine Issues related to FlowMachine Needs Review PR that is ready for review by a human ready-to-merge Label indicating a PR is OK to automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache cleanup fails if it hits a custom class
3 participants