Skip to content

bpo-23892: Introduce sys.implementation.opt_levels #9826

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

bpo-23892: Introduce sys.implementation.opt_levels #9826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f88e45
bpo-23892: Introduce sys.implementation.opt_levels
csabella Oct 12, 2018
8247004
Correction for refleak.
csabella Mar 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Doc/library/sys.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,11 @@ always available.
``cache_tag`` is set to ``None``, it indicates that module caching should
be disabled.

*opt_levels* is a tuple containing the available compiler-supported
optimization levels. See :pep:`488` for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that mentioning the possible values, and the relation to the "optimization" parameter of importlib.util.cache_from_source(), is perhaps warranted here (in addition to linking to PEP-488).

Copy link
Member

Choose a reason for hiding this comment

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

+1

The relationship between sys.flags.optimize and that "optimization" parameter (as well as the "optimize" parameter of compile()) reflects a gap in the solution presented by PEP 488. That needs to be resolved.


.. versionadded:: 3.8
Copy link
Member

Choose a reason for hiding this comment

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

3.9 now :)


:data:`sys.implementation` may contain additional attributes specific to
the Python implementation. These non-standard attributes must start with
an underscore, and are not described here. Regardless of its contents,
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/support/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ def forget(modname):
# It doesn't matter if they exist or not, unlink all possible
# combinations of PEP 3147/488 and legacy pyc files.
unlink(source + 'c')
for opt in ('', 1, 2):
for opt in sys.implementation.opt_levels:
unlink(importlib.util.cache_from_source(source, optimization=opt))

# Check whether a gui is actually available
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_sys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,7 @@ def test_implementation(self):
self.assertTrue(hasattr(sys.implementation, 'version'))
self.assertTrue(hasattr(sys.implementation, 'hexversion'))
self.assertTrue(hasattr(sys.implementation, 'cache_tag'))
self.assertTrue(hasattr(sys.implementation, 'opt_levels'))

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we should probably also make sure that no other required attrs exist. (We could parameterize this part at the same time.)

required = [attr for attr in dir(sys.implementation) if not attr.startswith('_')]
expected = ['name', 'version', 'hexversion', 'cache_tag', 'opt_levels']
self.assertCountEqual(required, expected)
# Make sure (best effort) that sys.implementation.__dir__() is correct.
for attr in expected:
    self.assertTrue(hasattr(sys.implementation, attr))

We should also make sure that sys.implementation.opt_levels is a tuple of only non-negative ints and alphanumeric strings.

self.assertIs(type(sys.implementation.opt_levels), tuple, sys.implementation.opt_levels)
for opt in sys.implementation.opt_levels:
    if type(opt) is int:
        self.assertTrue(opt.isalnum(), opt)
    else:
        self.assertIs(type(opt), int, opt)
        self.assertGreaterEqual(opt, 0)

version = sys.implementation.version
self.assertEqual(version[:2], (version.major, version.minor))
Expand Down
75 changes: 28 additions & 47 deletions Lib/zipfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2034,63 +2034,44 @@ def _compile(file, optimize=-1):
return False
return True

def _check_cache(pycache, file):
Copy link
Member

Choose a reason for hiding this comment

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

nice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I think even if this ends up not being in sys.implementation, these changes in zipfile still might be worth adding to simplify the code.

return (os.path.isfile(pycache) and
os.stat(pycache).st_mtime >= os.stat(file).st_mtime)

file_py = pathname + ".py"
file_pyc = pathname + ".pyc"
pycache_opt0 = importlib.util.cache_from_source(file_py, optimization='')
pycache_opt1 = importlib.util.cache_from_source(file_py, optimization=1)
pycache_opt2 = importlib.util.cache_from_source(file_py, optimization=2)

pycache_opt = [importlib.util.cache_from_source(file_py, optimization=opt)
for opt in sys.implementation.opt_levels]
if self._optimize == -1:
# legacy mode: use whatever file is present
if (os.path.isfile(file_pyc) and
os.stat(file_pyc).st_mtime >= os.stat(file_py).st_mtime):
if _check_cache(file_pyc, file_py):
# Use .pyc file.
arcname = fname = file_pyc
elif (os.path.isfile(pycache_opt0) and
os.stat(pycache_opt0).st_mtime >= os.stat(file_py).st_mtime):
# Use the __pycache__/*.pyc file, but write it to the legacy pyc
# file name in the archive.
fname = pycache_opt0
arcname = file_pyc
elif (os.path.isfile(pycache_opt1) and
os.stat(pycache_opt1).st_mtime >= os.stat(file_py).st_mtime):
# Use the __pycache__/*.pyc file, but write it to the legacy pyc
# file name in the archive.
fname = pycache_opt1
arcname = file_pyc
elif (os.path.isfile(pycache_opt2) and
os.stat(pycache_opt2).st_mtime >= os.stat(file_py).st_mtime):
# Use the __pycache__/*.pyc file, but write it to the legacy pyc
# file name in the archive.
fname = pycache_opt2
arcname = file_pyc
else:
# Compile py into PEP 3147 pyc file.
if _compile(file_py):
if sys.flags.optimize == 0:
fname = pycache_opt0
elif sys.flags.optimize == 1:
fname = pycache_opt1
else:
fname = pycache_opt2
arcname = file_pyc
for pycache in pycache_opt:
if _check_cache(pycache, file_py):
# Use the __pycache__/*.pyc file, but write it to the legacy pyc
# file name in the archive.
fname = pycache
arcname = file_pyc
break
Copy link
Member

Choose a reason for hiding this comment

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

This "break" implies that the order of sys.implementation.opt_levels is extremely important. In a world where we deal only in sets of optimizations represented as increasing interger "levels" thats fine, but PEP 488 suggests that's not necessarily where we're headed. We should discuss this more on the BPO issue. :)

If we end up sticking with opt levels as currently used then the importance of order should be made clear in the docs.

else:
fname = arcname = file_py
# Compile py into PEP 3147 pyc file.
if _compile(file_py):
fname = pycache_opt[sys.flags.optimize]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this catch the IndexError and fall back fname = arcname = file_py?

FWIW, this code implies that sys.flags.optimize is guaranteed to be suitable as a tuple index (but such a guarantee does not exist).

Also, PEP 488 opens that door for dealing with sets of optimizations (either as a container of strings/ints or as a hash of them). This code would fall apart then.

We can discuss this more on the BPO issue.

arcname = file_pyc
else:
fname = arcname = file_py
else:
# new mode: use given optimization level
if self._optimize == 0:
fname = pycache_opt0
arcname = file_pyc
else:
arcname = file_pyc
if self._optimize == 1:
fname = pycache_opt1
elif self._optimize == 2:
fname = pycache_opt2
else:
msg = "invalid value for 'optimize': {!r}".format(self._optimize)
raise ValueError(msg)
if not (os.path.isfile(fname) and
os.stat(fname).st_mtime >= os.stat(file_py).st_mtime):
arcname = file_pyc
try:
fname = pycache_opt[self._optimize]
except IndexError:
msg = "invalid value for 'optimize': {!r}".format(self._optimize)
raise ValueError(msg)
if not _check_cache(fname, file_py):
if not _compile(file_py, optimize=self._optimize):
fname = arcname = file_py
archivename = os.path.split(arcname)[1]
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2018-10-12-13-03-04.bpo-23892.ttyikM.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Introduce sys.implementation.opt_levels.
8 changes: 8 additions & 0 deletions Python/sysmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2232,6 +2232,14 @@ make_impl_info(PyObject *version_info)
if (res < 0)
goto error;

value = Py_BuildValue("(sii)", "", 1, 2);
if (value == NULL)
goto error;
res = PyDict_SetItemString(impl_info, "opt_levels", value);
Py_DECREF(value);
if (res < 0)
goto error;

#ifdef MULTIARCH
value = PyUnicode_FromString(MULTIARCH);
if (value == NULL)
Expand Down