Set pytest import mode to importlib

[ebsmothers]

Context

#445 broke tune cli. Ref:

However, neither local runs of pytest nor our CI caught this import error.

This is because of some default pytest behavior modifying sys.path. Deep in pytest internals, the import_path method is called here to import any modules necessary to access conftest.py. Note that the default value of the importmode variable here is ImportMode.prepend (different import modes are explained here). The upshot is that here the variable pkg_root gets prepended to sys.path. I inspected this variable in my local pytest run, and it is in fact /data/users/ebs/torchtune (i.e. the package root).

Why is this bad? Well now that recipes are not importable, appending the package root to sys.path before running pytest is bad. Because it means pytest will always be able to find stuff in recipes/, even though it shouldn't.

Changelog

• To fix this I updated our pyproject.toml file to override the prepend import behavior with importlib. (Maybe obviously) this uses importlib instead and does not touch sys.path.

Test plan

We actually still have some erroneous recipes imports floating around (e.g. here in test_configs.py)

I locally ran

pytest tests/recipes/configs/test_configs.py
...
______________________________________________________________________________ ERROR collecting tests/recipes/configs/test_configs.py _______________________________________________________________________________
ImportError while importing test module '/data/users/ebs/torchtune/tests/recipes/configs/test_configs.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/recipes/configs/test_configs.py:10: in <module>
    from recipes.full_finetune import FullFinetuneRecipe
E   ModuleNotFoundError: No module named 'recipes'

Prior to modifying test_configs.py we can now see the import error in our CI (link to failed job):

After 0174701, test_configs.py is removed, and CI should (hopefully) be green.

Fixes #459

[netlify]

✅ Deploy Preview for torchtune-preview ready!

Name	Link
🔨 Latest commit	0174701
🔍 Latest deploy log	https://app.netlify.com/sites/torchtune-preview/deploys/65e936cc9086c70008fba460
😎 Deploy Preview	https://deploy-preview-460--torchtune-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[joecummings]

You're amazing

[rohan-varma]

Awesome investigation and thanks for the thorough explanation!

Very noob q: should we try to get test_configs actual unittest working + running in CI, instead of deleting it? Seems quite valuable to ensure all our checked in configs are well formed.

[ebsmothers]

Very noob q: should we try to get test_configs actual unittest working + running in CI, instead of deleting it? Seems quite valuable to ensure all our checked in configs are well formed.

@rohan-varma this is a very reasonable question. I am going to punt that to another change, mainly because (a) the test is already being skipped, (b) @RdoubleA has other config changes inflight (see #456), and (c) I claim having a well-functioning CI trumps any individual unit test. But I’ve created #466 for this.