Skip to content
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

Migrate from LFS #75

Merged
merged 91 commits into from
Nov 14, 2022
Merged

Migrate from LFS #75

merged 91 commits into from
Nov 14, 2022

Conversation

jncots
Copy link
Collaborator

@jncots jncots commented Oct 7, 2022

Currently the data tables in directory iamdata which are used by some models are located on LFS. This doesn't allow to test all models on CI. That should be changed by attaching binary files to release on github. The tables should be downloaded automatically by python when they are required.

This PR is opened to solve this problem.

@jncots jncots linked an issue Oct 7, 2022 that may be closed by this pull request
@jncots
Copy link
Collaborator Author

jncots commented Oct 7, 2022

Just for starters I add a couple of functions in util.py for downloading and checking database files. If the model needs database files, one should call the function util._check_db_file(remote_file, local_file, checksum) in init_generator. If the file doesn't exist or is corrupted, it downloads it from github.

@jncots
Copy link
Collaborator Author

jncots commented Oct 7, 2022

The code is taken from MCEq with minor changes.

@jncots
Copy link
Collaborator Author

jncots commented Nov 8, 2022

In the previous commits I was trying to build Pythia8 on Windows. There were a problem with dlfcn-win32 library, or to be exactly dlfcn.h could not be found. Now I fixed that problem. Unfortunately, now the built library file is not loaded. That is more serious...

@jncots
Copy link
Collaborator Author

jncots commented Nov 10, 2022

I excluded Pythia8 on Windows from building and from tests.

I leave here the notes about building problem with Pythia8 on Windows.

  • The problem with dlfcn.h file and corresponding library was solved by downloading, installing dlfcn-win32 library. See it test.yml workflow.
  • After installation of dlfcn the library is built, but not loaded with the error ImportError: DLL load failed while importing _pythia8: The specified module could not be found. By inserting by hand flag -static -Wl,-allow-multiple-definition for _pythia8 and -static for libpythia8 at the linking stage I got a .pyd file which doesn't produce an error. But it is loaded and works but only when python script is run under debugger. If it is run without debugger, the program just stops at the stage of library loading.
  • Another strange phenomenon: I tried to build a single _pythia8.pyd file by putting all sources in a single target (without separate building _pythia8.pyd and libpythia8). For some reasons _pythia8.pyd still searches libpythia8.dll.

@jncots
Copy link
Collaborator Author

jncots commented Nov 10, 2022

PR can be closed if no other requirements

@jncots
Copy link
Collaborator Author

jncots commented Nov 14, 2022

The main goal of the PR - moving from LFS - has been reached. Another task - build and pass tests on Windows - has been done partially. There are problems with two models Pythia8 and UrQMD. Both models don't load with errors related to dlls. Currently I removed these models from building and tests on Windows.

@HDembinski do you have any idea why Pythia8 build fails on Windows?

I propose to close this PR. @afedynitch, @HDembinski what do you think?

Copy link
Member

@afedynitch afedynitch left a comment

Choose a reason for hiding this comment

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

There are still a few things, which could be improved. Like removal of files or paths from the config because they are not used anymore.
One issue I'd like to see fixed is that the paths to the files in DPMJET, these paths should class vars, so by subclassing one can easily overwrite them.

F2Py.cmake Show resolved Hide resolved
src/impy/models/dpmjetIII.py Outdated Show resolved Hide resolved
src/impy/models/dpmjetIII.py Outdated Show resolved Hide resolved
src/impy/models/phojet.py Outdated Show resolved Hide resolved
@HDembinski
Copy link
Collaborator

HDembinski commented Nov 14, 2022

One issue I'd like to see fixed is that the paths to the files in DPMJET, these paths should class vars, so by subclassing one can easily overwrite them.

I second that.

Edit: The remote path as a string can be a classvar, but the call to _cached_data_dir must be in __init__, just like it is now.

@HDembinski
Copy link
Collaborator

The main goal of the PR - moving from LFS - has been reached. Another task - build and pass tests on Windows - has been done partially. There are problems with two models Pythia8 and UrQMD. Both models don't load with errors related to dlls. Currently I removed these models from building and tests on Windows.

I am fine with that solution. Ideally, Pythia8 and UrQMD should not be importable from impy.models if they are not available on a platform. I am not sure about the easiest and most efficient way to achieve this at the moment. We should not add complicated code to impy to handle this edge case just for a negliglbe user base that runs Windows.

@HDembinski do you have any idea why Pythia8 build fails on Windows?

No, and I don't think it is worth any more effort in making Pythia8 work on Windows. If Pythia8 is not officially supported on Windows by the Pythia8 authors, we should not try to make it work (unless it was easy to do). The user base on Windows is neglgible.

I propose to close this PR. @afedynitch, @HDembinski what do you think?

I am ok with that.

Copy link
Collaborator

@HDembinski HDembinski left a comment

Choose a reason for hiding this comment

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

I think all of Anatoli's last requests are met.

@afedynitch
Copy link
Member

Yea, let's merge.

On the Pythia8 Windows mystery: it's not related to Pythia8's code but rather the whole python bindings, dynamic libs vs static libs stuff. It's a bit mysterious.

For UrQMD, I agree that the user base is tiny and it's not worth putting any time into it.

@jncots jncots merged commit cf83a80 into main Nov 14, 2022
@HDembinski HDembinski deleted the migrate_from_lfs branch February 8, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attach binary files to release
3 participants