dftbplus icon indicating copy to clipboard operation
dftbplus copied to clipboard

Added API functions for ASI API implemenation. Including import and e…

Open PavelStishenko opened this issue 2 years ago • 9 comments

This PR is a rebased version of the PR #1093 . It includes all the work done to enable ASI API implementation. Mainly C API extension to include export and import of the Hamiltonian, Overlap, and Density matrices.

PavelStishenko avatar Apr 04 '23 18:04 PavelStishenko

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (21d2e0a) 69.81% compared to head (99afc33) 69.81%.

:exclamation: Current head 99afc33 differs from pull request most recent head fa83b9a. Consider uploading reports for the commit fa83b9a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1217      +/-   ##
==========================================
- Coverage   69.81%   69.81%   -0.01%     
==========================================
  Files         227      228       +1     
  Lines       42543    42382     -161     
==========================================
- Hits        29700    29587     -113     
+ Misses      12843    12795      -48     
Files Coverage Δ
src/dftbp/api/mm/capi.F90 91.11% <100.00%> (+8.44%) :arrow_up:
src/dftbp/api/mm/mmapi.F90 80.54% <100.00%> (+2.32%) :arrow_up:
src/dftbp/dftbplus/initprogram.F90 82.86% <ø> (+0.13%) :arrow_up:
src/dftbp/dftbplus/main.F90 90.86% <100.00%> (+0.01%) :arrow_up:
src/dftbp/dftbplus/mainapi.F90 83.97% <100.00%> (+3.61%) :arrow_up:
src/dftbp/api/mm/apicallbackc.f90 90.00% <90.00%> (ø)
src/dftbp/dftbplus/apicallback.F90 96.00% <96.00%> (ø)

... and 16 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 18 '23 11:04 codecov[bot]

I believe that all suggestions are accepted or fixed.

PavelStishenko avatar Apr 19 '23 12:04 PavelStishenko

@PavelStishenko I've made a PR into your branch (instead of listing lots of minor changes). As mentioned in that PR, there are then a few things to discuss on top of this, mostly to lock down users from performing broken calculations if the matrices are imported.

bhourahine avatar May 09 '23 21:05 bhourahine

OK, just to keep them, listing the comments from PR#1 into this:

  • the 'invokeX' method isn't too informative as a name for the API calls, is it an exchange, import or export of quantities?
  • Please export double precision values for the expdmhs autotest.tag data.
  • Cases that will fail to work correctly will need to be locked down (mostly property evaluations using the sparse S, as this isn't constructed, but this could be constructed given the k-dependent S, but we'll need to know the spatial cut-off of matrix elements).
  • If the callback gets registered, various invoked modes of the code will need to be locked out of being used (mostly excited state methods).

bhourahine avatar May 20 '23 15:05 bhourahine

@PavelStishenko Are there any security issues for the current structure of leaving an open set of calls in the binary? There don't seem to be any obvious buffer overrun possibilities. I guess we can at least add a flag inside the DFTB+ input to disable the callback register and only build it in if the api is enabled.

bhourahine avatar Jun 05 '23 09:06 bhourahine

@PavelStishenko Are there any security issues for the current structure of leaving an open set of calls in the binary? There don't seem to be any obvious buffer overrun possibilities. I guess we can at least add a flag inside the DFTB+ input to disable the callback register and only build it in if the api is enabled.

If I understand your concerns correctly, then for sure there are no security issues due to API functions. This kind of API can be used only when the DFTB+ library is already in the address space of the process that is able to do API calls. In that case the process is capable to do whatever it wants with the library. It is by design. Of course, if there is a bug in the process that loads and uses the DFTB+ library, then it can corrupt anything in its address space, including DFTB+ data, but I don't think that it is a problem of the library.

Perhaps it is possible to compile API support conditionally, but I don't see the problem that is supposed to be solved in that way. It will require more efforts to support and test such code. And definitely will take a time to implement.

PavelStishenko avatar Jun 05 '23 13:06 PavelStishenko

The API can be conditionally compiled, but not the ASI parts. I think it is also safer if the user input can also turn the feature on and off for enabled binaries (this is what i-PI does). I'll make a PR into your branch with the changes for this (rebased on the current main code) in a few minutes.

bhourahine avatar Jun 05 '23 13:06 bhourahine

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Aug 14 '24 03:08 stale[bot]