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

Draft for plugin loading mechanism #1449

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

terminationshock
Copy link
Contributor

This is a first draft of a plugin loading mechanism. An external C library implements certain functions (see dftbplus_plugin.h) and will be loaded in DFTB+ at startup. This example here has been created for getSKIntegrals.
Both the implementation of the plugin as well as the C binding in DFTB+ are quite lightweight. Further features are needed, like a geometry update notification.
At the moment, I am happy to hear comments and critizism.

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

Attention: Patch coverage is 88.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 70.86%. Comparing base (22572cb) to head (f6a5d57).
Report is 43 commits behind head on main.

❗ Current head f6a5d57 differs from pull request most recent head 9d4b93a. Consider uploading reports for the commit 9d4b93a to get more accurate results

Files Patch % Lines
src/dftbp/plugins/plugin.F90 83.33% 3 Missing ⚠️
src/dftbp/plugins/plugin.c 90.00% 2 Missing ⚠️
src/dftbp/dftbplus/initprogram.F90 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1449   +/-   ##
=======================================
  Coverage   70.86%   70.86%           
=======================================
  Files         231      233    +2     
  Lines       44203    44236   +33     
=======================================
+ Hits        31325    31349   +24     
- Misses      12878    12887    +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bhourahine
Copy link
Member

bhourahine commented May 2, 2024

@terminationshock thoughts so far:

  • Eventually this will need some form of CI for plugins from outside of the test directory cmake system. So probably a cmake finder mechanism that can be used by others
  • Multiple plugins eventually...
  • in the parser, logically does the plugin belong in options {} ? Possibly a Plugins {} block?
  • There is some overlap with External model interface #1420, so we should unify. Features from there that are probably relevant
    • plugins returning an announcement string and/or version number for what they are/do/which one
    • Consistency checking/announcement for what a plugin provides, and hence what code functionality can be used with it
  • Features more generally: modify parser path depending on plugins loaded
  • For current draft, once it gets more finalised: documentation and a changelog entry.

We should document all of this up and try to get buy in from other similar codes.

@aradi any thoughts? In particular, you had ideas about an internal registry or features, and this should be planned to connect with it smoothly (possibly with extra methods and parser sections turning on/off with plugins). Could we push this far enough to, for example, replace the sockets code with loadable external connector plugin(s).

@awvwgk
Copy link
Member

awvwgk commented May 8, 2024

Would it be possible to make a case study using an existing external library to implement it via the plugin mechanism? In this case many use cases would be covered which need to be supported for having a proper plugin working with DFTB+. An example could be tblite which is almost completely integrated via two main API routes (computing of integrals for Hamiltonian and exchange of charges and potentials), however still needs a significant change in the DFTB+ main code to work compared with SK based DFTB. Another case which is a bit simpler could be dispersion corrections like DFT-D3 or MBD (note that the latter requires quite some custom work).

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.

None yet

3 participants