openlibrary icon indicating copy to clipboard operation
openlibrary copied to clipboard

Fix copydocs: Move sys path insert up to fix import

Open Suhas-13 opened this issue 3 years ago • 5 comments

Closes #6790

Fix #6790 by moving sys.path.insert up to allow import call to work

Technical

Testing

Run copydocs.py on example work

Screenshot

Stakeholders

@jimchamp

Suhas-13 avatar Aug 02 '22 06:08 Suhas-13

An alternative approach to this is via the use of scripts/_init_path.py

Usage: https://github.com/internetarchive/openlibrary/search?q=_init_path

The nice thing about using scripts/_init_path.py is that linters like flake8 and isort accept it.

cclauss avatar Aug 02 '22 16:08 cclauss

Thanks @cclauss! That worked for me when I tested locally.

jimchamp avatar Aug 03 '22 00:08 jimchamp

The python test seems to fail when I use _init_path @jimchamp

Suhas-13 avatar Aug 03 '22 12:08 Suhas-13

I'm going to defer to @cclauss and @cdrini for advice about fixing the failing test.

Do either of you know how to resolve the following?

ImportError while importing test module '/home/runner/work/openlibrary/openlibrary/scripts/tests/test_copydocs.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.9.13/x64/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
scripts/tests/test_copydocs.py:1: in <module>
    from ..copydocs import copy, KeyVersionPair
scripts/copydocs.py:11: in <module>
    import _init_path
E   ModuleNotFoundError: No module named '_init_path'

jimchamp avatar Aug 04 '22 01:08 jimchamp

@Suhas-13 can we see if copydocs works with the new revised instructions which include setting the PYTHONPATH? It's possible we can avoid having to add this change, which would be great! (Apologies if we sent you down a rabbit hole)

https://github.com/internetarchive/openlibrary/wiki/Loading-Production-Book-Data

@cdrini maybe it's possible for us to add PYTHONPATH=. to our docker-compose setup (at least for dev)

mekarpeles avatar Aug 08 '22 20:08 mekarpeles

I was able to import records using the updated copydocs instructions. I'm going to go ahead and close this PR and the linked issue. @Suhas-13, thanks for creating this PR! Even though it didn't get merged, your efforts have helped us troubleshoot and solve the issue.

jimchamp avatar Aug 12 '22 21:08 jimchamp