Public API #704
Comments
Yes, I think we should decide on what is a part of the API. Or what is not.
Well, you could import anything, including underscore-prefixed names. I think we should be conservative: the current API is anything that is imported into the global mpmath namespace (i.e. with Or backward compatibility will block even simple changes. An example: #414. Clearly,
On another hand, instead of introducing these unused symbols yet in another private module, you could just import the
In fact, this converter was restored in sympy/sympy@751502e by you:) Do you realize why? I think it could be removed. |
With 1.3.0 there are 400 names there and the difference is: Probably only rational is something that downstream code might be expected to use from there.
It is not hard to change sympy to work with all versions of mpmath but we also need to remember that other code might be broken by these things. Also the incompatibility between older sympy versions and newer mpmath versions cannot be resolved easily because it is not possible to add a dependency version constraint to existing releases.
With all changes backwards compatibility needs to be considered. There can be good reasons for breaking compatibility but it should not be done without any good reason. The |
I think it should be perfectly clear that the code that "only intended for internal use" is, well, "only intended for internal use".
I'll add
It will be better, if we first could factor out something, that we could call API. |
This code was added in sympy/sympy@1edecf7 in sympy/sympy#14348 |
|
I saw this, but it doesn't explain why SymPy uses this class. Another (only?) use case is the mpelements.py. This block could trivially be changed to use Fraction's. Unfortunately, proposed above solution will not work: diff --git a/mpmath/rational.py b/mpmath/rational.py
new file mode 100644
index 0000000..7fbc2ca
--- /dev/null
+++ b/mpmath/rational.py
@@ -0,0 +1 @@
+from .libmp import MPQ as mpqThe old mpq class has a slightly different interface (with p/q attributes, instead of numerator/denominator, despite claiming to support the numbers.Rational interface). The only option I see is restoring this internal class as is and then deprecate it. This sounds rather silly for me: usually "internal use" interfaces by definition could be broken without a warning... |
I think that if it is not needed any more by mpmath itself then it is fine to remove it. If there are cases though where mpmath might return an MPQ then there should be some public way for downstream code to access the MPQ type. SymPy's use of mpmath's mpq is just to convert it into something else. It is only relevant in so far as some mpmath interface might return an mpq that would need to be converted. I'm not sure how or when that would happen so I don't know if it is really necessary. Probably SymPy has no need for this and could just remove it.
We have to be careful about this. A general problem in Python is that it is not often clear what is internal vs public and by default everything is made publicly accessible by the language itself. Relying on documentation to clarify this is problematic because unless all docs consistently describe what is or is not public then we cannot really expect that someone would even look at the docs to discover it. |
I doubt there are.
It's just a numbers.Rational implementation (Fraction or gmpy2.mpq). BTW, you can import this type from the backend module (like MPZ).
I don't think there is a real need for this.
My proposal above is coming from the PEP 8: "All undocumented interfaces should be assumed to be internal." (c) I'm open to other suggestions. I realize, that some projects (mostly sympy;)) use much more "private" and/or low-level API of the mpmath (e.g. functions from libmp and so on) and we must take this situation into account. (I hope it's temporary, because SymPy's mpmath integration looks as a half-finished to me.)
That's not true, e.g. PEP 8 has leading underscore special convention as a weak "internal use" indicator. Another mechanism, suggested by PEP 8 is using |
|
With #703 the issue with mpnumeric import should be fixed. I think, that other stuff (related to using Let's rename this issue somehow, to reflect the fact it's about the mpmath API. |
The PEP 8 has this guideline: "Documented interfaces are considered public, unless the documentation explicitly declares them to be provisional or internal interfaces exempt from the usual backwards compatibility guarantees. All undocumented interfaces should be assumed to be internal." There is a slight uncertainty in what we should treat as "a documented interface", i.e. if it just has a docstring or if it was mentioned also in the sphinx docs. I think we should use the second version. With a simple GH search is seems that the most heavy user of In a long term, I think it's a good idea to rewrite code in dependents like the Sympy. The only good reason to not use high-level classes like mpf is #696. But some imports could be removed right now. I.e. why depend on the |
I agree. I would be happy to do that immediately in SymPy if we had a clear list of what should not be used. Currently: Mostly it is:
Removing those we are left with: There are three places that import lots of things. In evalf we have In numbers.py we have partitions.py has mpelements.py has The most invasive thing is subclassing Some things could easily be removed like I'm not sure why there is a subclass of Otherwise do you see anything obvious that should be considered private to mpmath? |
All mpmath.libmp.* stuff. Why not use high-level interfaces ( |
Okay, I agree with this. However looking at it a little bit this is not quite as simple as I hoped. The highest level mpmath APIs all use a global precision in the mpmath module context objects. At minimum SymPy should be using its own |
Yes, that's an issue and we have #683.
The domains module code already does this, but with a some reinvention of the MPContext class. |
|
With some refactoring of mpmath.libmp imports in the diofant (diofant/diofant#1357 and diofant/diofant#1361) I got this list: All imports are from I would expect same situation for the SymPy. Script to find importsIt should be possible to add few more cases to workaround various import patterns in the Sympy.import ast
import os
import sys
sys.setrecursionlimit(10000) # for sympy
libmp_imports = set()
class ImportVisitor(ast.NodeVisitor):
def visit_ImportFrom(self, node):
if node.module == 'mpmath.libmp':
for n in node.names:
libmp_imports.add(n.name)
for root, sdirs, files in os.walk(sys.argv[1]):
for fname in files:
if fname.endswith('.py'):
with open(os.path.join(root, fname), 'r') as file:
source = file.read()
tree = ast.parse(source)
ImportVisitor().visit(tree)
print(sorted(libmp_imports)) |
|
Thinking about this a bit I think that what SymPy really needs is pure functions like If the functions in libmp are to be considered private then I don't see what the public API is that replaces them. |
That's true in the current shape. But we have #683 for this. So, I don't see here big obstacles to use (eventually) a high-level interface (context & mpf/mpc classes).
And that's definitely possible, see e.g. gmpy2. I think this kind of high-level API is better.
No. But I definitely would like to restrict this part of the API to some minimum, based on requirements of major its consumers (SymPy). |
|
It looks like you are preparing for a new release of mpmath. Currently mpmath 1.4.0 is incompatible with the latest release of sympy (1.12) that is available on PyPI so an mpmath release right now would break sympy. I am making changes to sympy to account for this (sympy/sympy#26269) and aim to put out a sympy 1.13 release soon. I think though that before there is an mpmath release one or (ideally) both of these things are needed:
Both sympy and sage reach deeply into mpmath (see also #727 (comment)). It would be good to reduce that but also it would be good for a new release of mpmath not to have changes that are known to be breaking for at least some downstream projects. It seems likely that other projects would also be affected by these changes as well. |
No. It's far, far away... Some issues I just can't solve alone, e.g.: #709 But I think it's a good idea to do alpha release.
So far, we should restore some code for the private rational module and |
The only other thing I noticed was a change to parsing that I accounted for in sympy/sympy@eef227f but that was just to reject certain string inputs. I'm not sure how likely it is that other projects might depend on these things besides the obvious ones. It seems likely though that if mpmath goes from being relatively dormant to making more active changes then some things might pop out. |
|
I'm not sure if some people are now picking up the mpmath prerelease somehow (sympy/sympy#26273). |
|
It looks correct on PyPI to me, so probably people are using
|
Or add a specific version requirements. But not in this case, unfortunately.
People here are using ancient packaging techniques (setup.py install, etc). These doesn't ignore pre-release (per PEP 440) versions. I think this issue is relevant: pypa/setuptools#855. You also could reproduce that with a simple setup.py (and you will see a lot of DeprecationWarning's!): from setuptools import setup
setup(name='a',
version='0.1',
#install_requires = ['mpmath<=1.3.0'],
install_requires = ['mpmath'],
packages = ['a'])Commented out line should help sympy people to solve sympy/sympy#26273. |
#769 will include some compatibility code for this. I did basic local tests: sympy v1.12 imports, pass tests in sympy/core/tests/test_numbers.py and sympy/core/tests/test_sympify.py, pickling - works for Float's. |
|
It seems, this is the libmp namespace, used both SymPy (since v1.12) and latest Diofant ( SymPy also uses private names: Lets start from the following namespace of mpmath.libmp: script find-libmp.pyimport ast
import os
import sys
import mpmath
sys.setrecursionlimit(10000) # for sympy
libmp_imports = set()
class ImportVisitor(ast.NodeVisitor):
def __init__(self):
self.names = set()
def visit_ImportFrom(self, node):
# from mpmath.libmp import foo, bar
if node.module == 'mpmath.libmp':
for n in node.names:
libmp_imports.add(n.name)
if node.module and node.module.startswith('mpmath.libmp.'):
# from mpmath.libmp.libmpf import foo
for n in node.names:
libmp_imports.add(n.name)
m = node.module
m = m.replace('mpmath.libmp.', '')
assert '.' not in m
libmp_imports.add(m)
# from mpmath import foo, bar, libmp
if node.module == 'mpmath':
for n in node.names:
if n.name == 'libmp':
self.names.add(n.asname if n.asname else n.name)
def visit_Import(self, node):
# import mpmath.libmp
# import mpmath.libmp as libm
# import mpmath.libmp.libmpf
# import mpmath.libmp.backend as b
for n in node.names:
if n.name == 'mpmath.libmp':
self.names.add(n.asname if n.asname else n.name)
elif n.name.startswith('mpmath.libmp.'):
self.names.add(n.asname if n.asname else n.name)
def visit_Attribute(self, node):
v = node.value
if isinstance(v, ast.Name):
if v.id in self.names:
a = node.attr
assert a in dir(mpmath.libmp)
libmp_imports.add(a)
if isinstance(v, ast.Attribute):
if isinstance(v.value, ast.Name):
if v.value.id == 'mpmath' and v.attr == 'libmp':
a = node.attr
assert a in dir(mpmath.libmp)
libmp_imports.add(a)
for root, sdirs, files in os.walk(sys.argv[1]):
for fname in files:
if fname.endswith('.py'):
with open(os.path.join(root, fname), 'r') as file:
source = file.read()
tree = ast.parse(source)
ImportVisitor().visit(tree)
libmp_imports -= {'backend', 'gammazeta', 'libelefun', 'libhyper',
'libintmath', 'libmpc', 'libmpf', 'libmpi'}
print("mpmath.libmp imports:")
print(set(sorted(libmp_imports)))
print("missing in latest namespace:")
print(set(sorted(_ for _ in libmp_imports if _ not in dir(mpmath.libmp)))) |
|
A new error for SymPy and mpmath master: This one is coming from dcc591c (gh-792). In SymPy mpmath's |
Good idea. After all, this class seems to be a private one. It's not documented or exported to the global namespace.
We could use ctx.isinf() insted. Or move isfinite method. |
Since 3414c6e from gh-701 import sympy fails:
The
mpnumericclass is the base class for_mpfand_mpcand is used by sympy for detecting when an object is an instance ofmpformpc. This is needed since_mpfand_mpcseem to be designated private andmpfandmpcare dynamically generated types (gh-696, gh-532).The fix for that is straight-forward:
However then a new error arises:
This is because the
mpmath.rationalmodule was removed in gh-691. I'm not sure if there ever is a reason that sympy would need to convert an mpmath mpq. If there is a reason though then it is not clear what the replacement should be since gh-691 describes it as private.This is enough to make
import sympywork:I wonder more generally though what should be expected in terms of backwards compatibility here. Since mpmath has not changed much in a long time there will be a lot of downstream code that depends on being able to import different things from different modules and it is not clear exactly what should be considered public or private. In gh-691 then
mpqtype is described as being "private" but actually it could be imported in a way that does not look like accessing anything private:It is not hard to make changes in sympy that would accommodate these changes in mpmath but it does make me wonder how much other code downstream from mpmath might be broken by these rearrangements of what is publicly importable. It can be difficult to see exactly how some change affects what is importable because things might even be implicitly imported (like the
mpmath.rationalmodule which was not explicitly imported inmpmath/__init__.py).The text was updated successfully, but these errors were encountered: