Fixed #1066 Permanently Remove Recompile

[seanlaw]

Pull Request Checklist

Below is a simple checklist but please do not hesitate to ask for assistance!

• Fork, clone, and checkout the newest version of the code
• Create a new branch
• Make necessary code changes
• Install black (i.e., python -m pip install black or conda install -c conda-forge black)
• Install flake8 (i.e., python -m pip install flake8 or conda install -c conda-forge flake8)
• Install pytest-cov (i.e., python -m pip install pytest-cov or conda install -c conda-forge pytest-cov)
• Run black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./ in the root stumpy directory
• Run flake8 --extend-exclude=.venv ./ in the root stumpy directory
• Run ./setup.sh dev && ./test.sh in the root stumpy directory
• Reference a Github issue (and create one if one doesn't already exist)

[seanlaw]

@NimaSarajpoor All tests are passing locally. Basically, I've completely removed the need to need a recompile function. Can you please find some time to review? Thanks in advance

Update

It looks like the test_precision.py::test_snippets test is failing. @NimaSarajpoor Do you have any idea why that might be happening? The snippets stuff seems to be somewhat unstable and causing frequent failed tests 😢

[codecov]

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 97.30%. Comparing base (bbc97e4) to head (80446fc).

Files with missing lines	Patch %	Lines
stumpy/fastmath.py	16.66%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1067      +/-   ##
==========================================
- Coverage   97.33%   97.30%   -0.03%     
==========================================
  Files          93       93              
  Lines       15219    15212       -7     
==========================================
- Hits        14813    14802      -11     
- Misses        406      410       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[NimaSarajpoor]

@seanlaw
Thanks for the update. I will look into it today.

[NimaSarajpoor]

@seanlaw
I think the issue is that calling the function snippet in the second attempt in tests/test_precision.py::test_snippets results in the same result as the first attempt. You can see it in this new version if you compare the outcomes of the first and second attempts. This is because the callers of the njit-with-new-fastmath-flag function (those callers that were involved when compiling stumpy.snippet) need to be recompiled. So, we need that cache._recompile() here.

---

If we decide to keep cache._recompile(), then I think we should add a comment in the test function to mention its purpose. I cannot think of an alternative approach here.

[NimaSarajpoor]

@seanlaw
I added a few comments for your consideration.

[NimaSarajpoor]

Is there any particular advantage in using try-except block? If not, what do you think about using a simple if-block?

if numba.config.DISABLE_JIT:  # pragma no cover
        # update fastmath if in JIT mode
        py_func = func.py_func  # Copy raw Python function (independent of `njit`)
        njit_signature = func.targetoptions.copy()  # Copy the `njit` arguments
        njit_signature.pop("nopython", None)  # Pop redundant `nopython` declaration
        njit_signature["fastmath"] = flag  # Apply new `fastmath` flag
        func = njit(py_func, **njit_signature)  # Assign `njit` function with new target
 
# Monkey-patch [njit] func into global space
 setattr(module, func_name, func)  

[seanlaw]

Is there any particular advantage in using try-except block? If not, what do you think about using a simple if-block?

If I think about "normal" usage, njit is ALWAYS "on". So it feels "wrong" to check numba.config.DISABLE_JIT as njit should be assumed to always be there. Attempting to access the fastmath attributes via try at least assumes that njit is always "on", the default behavior. Note that I would only consider try/except when the function is called infrequently/rarely.

I will think about this more.

[NimaSarajpoor]

If I think about "normal" usage, njit is ALWAYS "on". So it feels "wrong" to check numba.config.DISABLE_JIT as njit should be assumed to always be there.

Makes sense!! Thanks for helping me understand your view! The if-block might seem cleaner but it creates branching here which is not the case when we think about the regular scenario.

[NimaSarajpoor]

To remove cache._recompile(), we need to find an alternative approach for updating all relevant callers of the function core._calculate_squared_distance whose fastmath flag was updated in previous line.

[seanlaw]

Right. I did not appreciate this and did not capture this in my original code. I will explore more

[NimaSarajpoor]

Removing _recompile here means that _save only does cache._enable(), which does nothing if a njit function is already compiled. The reason that tests/test_cache.py::test_cache_save_after_clear is still passing is because both ref_cache and comp_cache are now empty.

stumpy/tests/test_cache.py

Lines 11 to 27 in bbc97e4

	def test_cache_save_after_clear():
	T = np.random.rand(10)
	m = 3
	stump(T, m)
	cache.save()
	ref_cache = cache._get_cache()
	cache.clear()
	# testing cache._clear()
	assert len(cache._get_cache()) == 0
	cache.save()
	comp_cache = cache._get_cache()
	# testing cache._save() after cache._clear()
	assert sorted(ref_cache) == sorted(comp_cache)

To have a better test, we should move up the first call of cache.save(), and put it just before calling the function stumpy.stump. This way we know that we have some files in ref. Then, the test in this version should fail and expose the issue to us.

However, when I made such change, I noticed that the tests are NOT failing still. The returned cache files is still an empty list. There is a deeper issue here. According to Numba's document:

If not defined, Numba picks the cache directory in the following order:
In-tree cache. Put the cache next to the corresponding source file under a pycache directory following how .pyc files are stored.

So, I turned on NUMBA_CACHE_DIR and noticed that it writes to the local git directory "stumpy", in the path ./stumpy/__pycache__, however, cache._get_cache() reads the files from the following path, which is where stumpy is installed.

.../miniconda3/envs/py310/lib/python3.10/site-packages/stumpy/__pycache__

and that's why it returns 0 files.

[seanlaw]

So, I turned on NUMBA_CACHE_DIR and noticed that it writes to the local git directory "stumpy", in the path ./stumpy/pycache, however, cache._get_cache() reads the files from the following path, which is where stumpy is installed.

This is unexpected behavior. I will look into this

[NimaSarajpoor]

@seanlaw

So, I turned on NUMBA_CACHE_DIR and noticed that ...

Correction: "I turned on NUMBA_DEBUG_CACHE"

[seanlaw]

Thanks @NimaSarajpoor!

I think the issue is that calling the function snippet in the second attempt in tests/test_precision.py::test_snippets results in the same result as the first attempt. You can see it in this new version if you compare the outcomes of the first and second attempts. This is because the callers of the njit-with-new-fastmath-flag function (those callers that were involved when compiling stumpy.snippet) need to be recompiled. So, we need that cache._recompile() here.

It's clear that I do not understand the importance of cache._recompile(). In my original example, I only considered a single function and NOT a caller/callee scenario. I'm going to play around with it a little bit more so that I can better understand the behavior.