fpylll
fpylll copied to clipboard
fplll
Loading strategies
After a from fpylll import *, load_strategies_json(BKZ.DEFAULT_STRATEGY) fails stating "Cannot open strategies file." ; because in fact it should be: load_strategies_json(BKZ.DEFAULT_STRATEGY_PATH + BKZ.DEFAULT_STRATEGY) or some such.
Notice that all of the comments/tests in the source point to the broken way ; only check_delta in src/fpylll/fplll/bkz_param.pyx tries to do the right thing by using strategy_full_path.
Arguably fplll's load_strategies_json should detect a relative filename and try to prepend its default path -- that would simplify matters.
In sagemath, sage/src/sage/matrix/matrix_integer_dense.pyx, the line kwds["strategies"] = load_strategies_json(BKZ.DEFAULT_STRATEGY) is hence broken.
Sigh. Since last release, I see it's now even worse! Commit a249d630debd8e96d536c37762f250d562aaec65 makes fpylll search for SAGE!? It's definitely an incorrect change: sagemath's fplll package should be configured so its DEFAULT_STRATEGY_PATH points to where default.json is installed, and fpylll should use just that!
Indeed, there should be no fooling around with Sage/SAGE_LOCAL here, the strategies info is in /usr/include/fplll/fplll_config.h: see macros
FPLLL_DEFAULT_STRATEGY_PATH and FPLLL_DEFAULT_STRATEGY (as @SnarkBoojum pointed out to me on a Debian list)
so it's a matter for a simple (say) Cython function to get these value for fpylll. Although it could be even better to store them in fplll.pc- then it could be obtained from Python very easily.
In fact, I think the best fix is a single-liner in fplll's load_strategies_json:
std::ifstream fs(filename); in its source should become std::ifstream fs(strategy_full_path(filename));
Reported on fplll here.
This issue on fpylll is now about purging the code from anything fancy about sagemath.
Happy to purge the Sagemath stuff from here: the suggested idea seems much better (mea culpa!)
I can't replicate the bug on my Debian box, but I'll trust from the reports that it exists and go from there.
In sagemath, sage/src/sage/matrix/matrix_integer_dense.pyx, the line
kwds["strategies"] = load_strategies_json(BKZ.DEFAULT_STRATEGY)is hence broken.
I think this is probably the best place to fix this. The biggest problem here is the lack of documentation on fp(y)lll side - how are these values, BKZ.DEFAULT_STRATEGY and BKZ.DEFAULT_STRATEGY_PATH, formed? Are they just taking what's in the fplll header (macros FPLLL_DEFAULT_STRATEGY and FPLLL_DEFAULT_STRATEGY_PATH), and the latter are always correct (since some version of fplll)? Or it's something more complicated?
The fix suggested by @SnarkBoojum might be the best for Debian, but it's suboptimal for mutiplatform Python libaries fpylll and Sage, which might rely on pre-compiled fplll, IMHO.
It's all quite messy. While talking about Debian 11's fplll, it's version 5.4.0, with FPLLL_DEFAULT_STRATEGY and FPLLL_DEFAULT_STRATEGY_PATH set to the correct path, and to default.json, respectively, in fplll/fplll_config.h. But on
e.g. Fedora 34, there is fplll 5.4.1, and there are no such macros in fplll/fplll_config.h. Instead, these macros as in fplll/defs.h, and they are set to "". Somehow, on Fedora 34 in Sage 9.5, BKZ.DEFAULT_STRATEGY is getting set to the correct full path to default.json (in /usr/share/fplll/strategies).
How this is happening, is hard to say without knowing the corresponding change in fplll in 5.4.1.
I'm trying to compile and run (with g++ foo.C -lfplll && ./a.out)
/* foo.C */
#include <iostream>
#include <fplll/fplll.h>
#include <fplll/bkz_param.h>
int main ()
{
std::cout << fplll::default_strategy_path() << "\n";
return 0;
}
on various systems. So on Fedora 34 (with unpatched (?) fplll 5.4.1) I get /usr/share/fplll/strategies, as it should be, even
though this value is not in the headers anywhere. So it seems to be hardcoded during build time.
On Debian 11, with fplll 5.4.0, I get /usr/share/libfplll7/strategies/, as it should be, and this same value is in the headers.
It's actually handled in fplll's Makefile.am, and I added a patch in Debian so it's versioned (make libfplll257 libfplll314 coinstallable). In 2017 according to git blame.
It seems to be a good patch, why isn't it upstreamed? The current situation with the headers in 5.4.1 is confusing at best.
Well, I forwarded it to upstream but it wasn't applied... perhaps you can propose again?
One way or another, I don't see the point of having these two macros set to "" in the header. Either they should be removed there completely, or kept with meaningful values. But "meaningful values" in an un-versioned header only makes sense if there is just one version of libfplll installed --- this is something I don't get about your patch.
There can be a singe libfplll-dev providing the headers at the same time, of course. But the binary packages will depend on the libfplll
If you have "foo" depending on libfplll6 and "bar" depending on libfplll7, then they can be installed at the same time because libfplll6 and libfplll7 care co-installable, and each can find its correct strategies file.
This is why my patch works.
libfplll6 and libfplll7 are co-installable, and each can find its correct strategies file.
yes, but FPLLL_DEFAULT_STRATEGY* macros will only be correct for one of them.
Those macros set the path at compile-time, while libfplll
(Just to make sure I understand the state of this issue) is your argument:
-
that the macros could be placed elsewhere during building, or
-
that we shouldn't have the macros at all, and instead store this information differently?
Is there anything extra that needs to be done here for this? From what I can tell this looks like a building issue, which I'm inclined to look into as a fplll issue (rather than an fpylll issue).
On Mon, 7 Feb 2022, 20:52 Dima Pasechnik, @.***> wrote:
why are these macros needed in the header file at all?
— Reply to this email directly, view it on GitHub https://github.com/fplll/fpylll/issues/221#issuecomment-1031908963, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7CASBDQLJNHHRFJU4NGDTU2AWIRANCNFSM5NTVQXLQ . You are receiving this because you were assigned.Message ID: @.***>
If fplll's api only allowed "load_strategies()", they wouldn't be needed.
But its "load_strategies(path + filename)", so the downstream developers need to access the default path and default filename to actually call that function with meaningful values.
From C++ it's all available via concatenating fplll::default_strategy_path() and fplll::default_strategy().
The values are baked in at build time.
to fix this in Sage, I opened https://trac.sagemath.org/ticket/33302
it's ready for review, please check.
The patch looks fine, but it's not entirely satisfying for fpylll (if there's an issue with paths in fplll we should fix it across all systems). Still, it's good this will work for Sagemath.
@malb what do you think? I'm thinking of #133 primarily, but also the examples that @dimpase has brought up for e.g Fedora.
I've just positively reviewed the Sage ticket. I've lost the plot, is there anything else we should do? Sorry and thanks all for your work on this!
is there anything else we should do?
as long as you don't break what you have now regarding BKZ.DEFAULT_STRATEGY_PATH and BKZ.DEFAULT_STRATEGY,
all is good and taken care of by the fix in Sage ticket.
I wanted to upload the latest fpylll to Debian, but had to add this patch, because it insisted on looking at the wrong place:
--- fpylll.orig/src/fpylll/fplll/bkz_param.pyx
+++ fpylll/src/fpylll/fplll/bkz_param.pyx
@@ -26,7 +26,7 @@
from fpylll.util cimport check_delta, check_pruner_metric
from cython.operator cimport dereference as deref, preincrement as inc
-from fpylll.config import default_strategy
+from fpylll.config import default_strategy_path, default_strategy
from .pruner cimport PruningParams
@@ -278,8 +278,8 @@
# catch. If it just so happens that we fail, we read the Sagemath environment variables and try
# to navigate to the default file relative to that directory.
- if not path.exists(filename) and filename == "default.json":
- filename = path.join(environ['SAGE_LOCAL'], 'fplll', 'strategies', 'default.json')
+ if not path.exists(filename):
+ filename = path.join(default_strategy_path, filename)
if not path.exists(filename):
raise FileNotFoundError("File '%s' not found."%filename)
handling of default.json was seriously messed up, by too eager changes done by fpylll devs to fix upstream a Sage issue. I recall untangling it a while ago.