refactor: proper TypeError handling in memoize decorator

[sabiroid]

SUMMARY

There are several issues with the memoize decorator:

• It also catches TypeErrors from within the memoized function
• In case of the key is not cacheable, it runs the memoized function for the second time, while having its result computed already
• It doesn't tell developers about possible fundamental problems with their memoizations (like, they accidentally provide a list to the memoized function and don't even know that memoization no longer works)
• Its intended behavior does not work anyway because of the if key in self.cache before the try/catch block raising TypeError already.

This PR fixes aforementioned issues:

• Calling the wrapped function outside of the try/catch block
• Returning the value variable regardless of the caching success status
• Logging the caching failure together with the stack trace - once for each failure
• Wrapping all cache operations in try/catch blocks.

As an alternative, we can also remove the try/catch altogether to throw explicitly. I won't be against that, especially since original behavior was already shadowed by uncaught dictionary reference.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

n/a

TESTING INSTRUCTIONS

I ran Superset locally and checked the memoization is still working. It is!

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #16074 (46ea8c1) into master (7ef97a5) will decrease coverage by 0.22%.
The diff coverage is 69.23%.

❗ Current head 46ea8c1 differs from pull request most recent head 4640d0d. Consider uploading reports for the commit 4640d0d to get more accurate results

@@            Coverage Diff             @@
##           master   #16074      +/-   ##
==========================================
- Coverage   76.86%   76.64%   -0.23%     
==========================================
  Files         995      995              
  Lines       52876    52882       +6     
  Branches     6720     6720              
==========================================
- Hits        40645    40531     -114     
- Misses      12005    12125     +120     
  Partials      226      226              

Flag	Coverage Δ
hive	?
mysql	81.58% <66.66%> (-0.02%)	⬇️
postgres	81.57% <66.66%> (-0.06%)	⬇️
presto	?
python	81.69% <66.66%> (-0.44%)	⬇️
sqlite	81.21% <66.66%> (-0.06%)	⬇️

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

Impacted Files	Coverage Δ
superset/utils/memoized.py	84.61% <66.66%> (-9.33%)	⬇️
...et-frontend/src/SqlLab/components/TableElement.tsx	88.75% <100.00%> (ø)
superset/db_engines/hive.py	0.00% <0.00%> (-82.15%)	⬇️
superset/db_engine_specs/hive.py	69.80% <0.00%> (-16.87%)	⬇️
superset/db_engine_specs/presto.py	83.47% <0.00%> (-6.49%)	⬇️
superset/views/database/mixins.py	81.03% <0.00%> (-1.73%)	⬇️
superset/connectors/sqla/models.py	88.08% <0.00%> (-1.67%)	⬇️
superset/db_engine_specs/base.py	87.98% <0.00%> (-0.39%)	⬇️
superset/models/core.py	89.61% <0.00%> (-0.26%)	⬇️
superset/utils/core.py	88.09% <0.00%> (-0.13%)	⬇️

---

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 7ef97a5...4640d0d. Read the comment docs.

[sabiroid]

thanks, updated PR with lint fixes. I haven't run the lints locally yet though since I hasn't figured out how to do that yet.

[nytai]

We're using the pre-commit python package to run checks. you can set this up to run automatically or you can run it manually against all staged files with pre-commit run (that's what I do).

[srinify]

@sabiroid FYI documentation on pre-commit / setting that up:

https://github.com/apache/superset/blob/master/CONTRIBUTING.md#git-hooks

[sabiroid]

I am actually not sure why this integration test is failing now. tox -e cypress succeeds for me locally. Also this failure doesn't seem related, even though you can never say that about integration tests for sure...

[sabiroid]

Update: I've set integration tests up. It actually fails 15 out of 48 tests on both this PR and its base revision, so this PR is not actually breaking any new tests.

[nytai]

rerunning CI

[sabiroid]

I've seen that all failed integration tests did so because of timeouts. So I updated the PR to re-throw the TypeError instead before computing the memoized function since this is what was effectively happening in trunk. Now there will be no changes in the decorator behavior, and e2e tests shouldn't run slower then before.

[mistercrunch]

💪 💪 💪 - congrats on your first PR @sabiroid !