-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
Make fastNLO converter part of the CLI #120
Conversation
I don't have a neat opinion on About linking, I will have a look as well, maybe someone wrote something somewhere about it (for sure such a generic statement is true, the problem is that the result of this query will be long). About the code itself this is exactly what I expected, and I'm very glad it is working. This is perfect for the first shot, but as you said having to use PineAPPL C API is odd, but I believe there is an easy solution for this: we should move most of the code to the Rust side, leaving more atomic functions in C++, rather than performing the full task on that side. I would suggest to move the minimal "fastNLO bindings" on a separate crate, inside this repository, as an external contribution and not directly part of PineAPPL CLI (but a dependency of course). Then we might think about publishing it or not on https://crates.io/, but from my point of view this comes at the end, so it's both far in the future and not needed for our main purpose. |
I changed it to
If you've got the time you could try and see whether you can reproduce my problem. You'll need to install the C API and fastNLO, and then make the following change: diff --git a/pineappl_cli/build.rs b/pineappl_cli/build.rs
index 0719736..a0d0061 100644
--- a/pineappl_cli/build.rs
+++ b/pineappl_cli/build.rs
@@ -46,8 +46,7 @@ fn main() {
println!("cargo:rustc-link-search={}", fnlo_lib_path);
// TODO: why do I have to link statically?
- println!("cargo:rustc-link-lib=static=fastnlotoolkit");
- println!("cargo:rustc-link-lib=z");
+ println!("cargo:rustc-link-lib=fastnlotoolkit");
// bridging code
bridge.file("src/import/fastnlo.cpp").compile("cxx-bridge"); That should link fastNLO dynamically, but I get lots of linking failures that complain about missing fastNLO symbols.
That's exactly the idea!
That may be a good idea, basically in the same spirit as my LHAPDF crate. |
pineappl_cli/build.rs
Outdated
} | ||
|
||
#[cfg(not(feature = "fastnlo"))] | ||
fn main() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two options for the feature gate:
- if you like to do what I proposed, and having a separate crate for "fastNLO bindings", than we need to do nothings right now (
build.rs
will be simply the build file of that crate, and we'll drop the feature gates, the only option will be spelled out inpineappl_cli/Cargo.toml
) - if we're going to keep everything in PineAPPL CLI, then you'll need a separate function for the time APPLgrid will join the party (and the feature gate we'll be placed not on
main()
but onfastNLO()
)
Personally, I still prefer option 1..
This afternoon I'll try to dynamically link. Trying what you put takes nothing, but I want at least a bit of time to investigate (it's unlikely that doing the same I'll obtain a different result). |
Codecov Report
@@ Coverage Diff @@
## master NNPDF/runcards#120 +/- ##
==========================================
- Coverage 89.63% 89.57% -0.06%
==========================================
Files 31 32 +1
Lines 3088 3090 +2
==========================================
Hits 2768 2768
- Misses 320 322 +2
Continue to review full report at Codecov.
|
I'm first reading references, starting from Linkage. Being about pure Rust, it's not very useful (it's useful for me to properly understand the rest).
In any case, this should not be our case, because it is building an executable.
They are both about possible failures and warnings, but mostly about Rust dependencies. However, I need to read the error messages first. |
Just to make some experience, I tried once all the possible installations:
|
I was joking: I read about I tried as well, and I'm trying to specify the The error message is still coming from the linker, and it's always the same, I report just the end:
so it seems like the flags are not arriving till there, so I'd keep try in this direction. No progress to report. |
P.S.: the infinite call to the compiler starts with So in principle the flags I believe to be needed are there, thus there is something wrong with them. |
Done in commit 15a91fe. |
This is correctly linking: #include <fastnlotk/fastNLOLHAPDF.h>
using namespace std;
int main()
{
auto file = fastNLOLHAPDF("h1.tab", "CT10");
return 0;
} with just g++ -g -Wall -o exp main.cpp $(fnlo-tk-config --ldflags)
# where the last expand to "-L/home/alessandro/.local/lib -lfastnlotoolkit" Indeed:
|
Concerning the linking error I'm afraid there's a problem with ordering: https://stackoverflow.com/questions/45135/why-does-the-order-in-which-libraries-are-linked-sometimes-cause-errors-in-gcc. I think for the time being we can ignore the error, and hopefully the situation will improve when the fastNLO bridge will be in a separate crate. |
I see. The lengthy (but helpful) SO answer taught me something, but now I start to believe that my limited C++ experience is going to kill me, so I'll give up for the time being (we can really investigate anew the moment we have a separate crate). Nevertheless, there is something mysterious going on in Cargo build, or in Another, more important, thing that I learned once more is that Cargo (in pure Rust) is much better than C/C++ builds, and it's really making my life easier... |
I could be wrong and there's an entirely different reason.
Absolutely, although meson more or less solved C++ builds for me. If you run the fastNLO example and finally compile with
It uses |
I never tried enough Meson, since I learned Rust it's hard for me to desire going "back" to C++. But it might happen that sooner or later I'll have to do it, and C++ is still better than Fortran (for general purpose programming, for linear algebra I actually like Fortran), and then I'll follow the advice and go with Meson (autotools are bloated and tiresome, and since my master thesis I've started hating CMake, it's not simple and it's poorly documented). Actually, some more info about are beloved build: I wanted to compare the size of the statically linked binary ( This is the result for the size of the
Actually, I've not been to compile in release mode the statically linked executable. Interestingly, in this case the problem is not linking cargo build --release --features=fastnlo (swapping the two arguments of |
I can confirm these numbers and interestingly the installed
I can confirm this. But that's just another sign that linking is broken, and it isn't necessarily tied to fastNLO. |
I don't know what it is exactly doing, but I can tell you that if you run: strip pineappl on the 3.7 MB executable, it becomes 1.9 MB. So I guess something similar... |
It seems that with Rust 1.59.0 I can link against the dynamic library, strange ... |
No idea why one has to transpose the indices, something isn't properly understood
This is now part of `pineappl import ...`
In commit c43977a I added the changes that we require for https://github.com/NNPDF/fktables/issues/27, and since commit 059491d the conversion should work properly. @alecandido could you please test this on dom? I can't access it right now. You need to install fastNLO as explained above and then install the CLI as follows:
You should then be able to run
to convert the fastNLO tables into PineAPPL grids; they will be placed in the same directory next to them in the applgrids repository. If you run
you should see |
@cschwan I would have done, but unfortunately I'm not able to access as well. Maybe dom is just down... |
There are more fastNLO tables in the official source tarball, and most of the grids work just fine. However, the following abort with 'could not access coefficient table':
My guess is that instead of having interpolation tables some orders are K-factors. Furthermore, there's a table 'NJetEvents_norm_1-12.pineappl.lz4' for which the bin results are wrong by a factor 500:
|
Unfortunately there is no actual documentation of fastNLO: there are the papers, but they do not document the code itself.
This is really a weirdly regular factor, I guess there should be a reasonable explanation. Something like bins' width again? |
You can run doxygen over the source code. But I'm reading the source code (the headers mostly) itself, as the documentation is often vague or even wrong in small instances. |
This is very likely. As this isn't a important right now I've opened a new Issue for it, NNPDF/pinecards#126. I'd like to merge this PR now, as everything that was working is working with the new converter. Do you agree or do you think there's something missing? You requested a Python API for the conversion, but for that I'll probably open a separate Issue or I'll add it into master directly. |
I perfectly agree on decoupling things: the goal of this PR is written in the name, and it's only about the CLI. As you said, it's definitely not worse than before, and now a proper part of the CLI. Let me try to compile it one last time, and I'll go quickly through the changes to see if everything looks alright. In any case, if I'll find something that is not strictly needed, I'll just open another issue, I'd like to merge as well. |
Thanks, let me know if it works, @alecandido! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything's fine, just a couple of comments.
This implements the suggestion by @alecandido discussed here: #115, following my outline given here: #115 (reply in thread). I tried to keep the changes minimal, and the biggest file is basically copied from the examples directory. You can view the difference here:
As you can see, I only had to rename the
main
function, and the argument parsing is now done byclap
inside the Rust CLI.The new subcommand works quite well, but it has the same dependencies as the C++ converter: fastNLO and the PineAPPL C API; the latter is a bit odd given that we use the C and the Rust API at the same time, and this must be improved in the future.
One odd point is that
build.rs
linksfastnlotoolkit
statically (withz
as a dependency), which I was forced to since linking dynamically it didn't work. I have to investigate why.Everything is hidden behind the feature
import-fastnlo
, so you have to compile the CLI using--features import-fastnlo
or--all-features
. @alecandido do you want to renameimport-fastnlo
tofastnlo
to keep it simple?