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

Autoload shared libraries #78

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

pviscone
Copy link
Contributor

When a conifer JSON model is loaded conifer is not able to load also the conifer_bridge.so shared libraries (if they were already produced).

This implies:

  1. The user must run .compile() every time
  2. Every time that the model is re-compiled a new shared library with a different timestamp is created and a new dict is appended in the metadata list in the model JSON

This PR tries to fix this behavior by looking for the shared library with the last timestamp present in the metadata. It looks for it in the same folder of the JSON if the is not specified by the user.
It works for both the xilinx and cpp backends.

@thesps thesps self-requested a review September 13, 2024 11:41
Copy link
Owner

@thesps thesps left a comment

Choose a reason for hiding this comment

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

Thanks for this feature, it makes sense to have. I’ve left code comments in review. You should also add some tests that touch this functionality in tests/test_save_load.py

conifer/model.py Outdated Show resolved Hide resolved
conifer/model.py Show resolved Hide resolved
conifer/model.py Show resolved Hide resolved
conifer/model.py Outdated Show resolved Hide resolved
conifer/model.py Outdated Show resolved Hide resolved
conifer/model.py Outdated Show resolved Hide resolved
@pviscone
Copy link
Contributor Author

pviscone commented Sep 13, 2024

Ok, I have applied all the comments.

  1. I created a load_shared_library method in ModelBase that does nothing but is overloaded in the backends that need to load a shared library.
  2. Now shared_library can be a bool or a string:
If True, the shared library will be looked for in the same directory as the JSON file, using the timestamp of the last metadata entry available
If False, the shared library will not be loaded
If a string, it could be:
    - path to the shared library to load for the model
    - path to the directory where to look for the .so file, using the timestamp of the last metadata entry available
    
No shared library will be loaded if a new configuration is provided

@thesps
Copy link
Owner

thesps commented Oct 1, 2024

I think the code changes for the library reloading are now good, but I'd like to see more tests:

  • split the test into some test functions that aim to test only this functionality
  • test some of the expected behaviour explicitly, not only the prediction results (e.g. check the name of the loaded library)
  • test different combinations of the arguments

@thesps
Copy link
Owner

thesps commented Nov 14, 2024

Did you get a chance to resolve the comments above RE more robust testing?

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.

2 participants