mdanalysis icon indicating copy to clipboard operation
mdanalysis copied to clipboard

Expose public facing Cython in libmda layer

Open hmacdope opened this issue 3 years ago • 2 comments
trafficstars

Is your feature request related to a problem?

As part of #3683, the libmda folder with its own __init__.pxd was introduced to provide a single place to expose public Cython APIs for interoperability. For example if external Cython code wishes to create a Timestep directly one can use:

from MDAnalysis.libmda cimport timestep

cdef ts = timestep.Timestep(...)

This is similar to the strategy used in Cython itself whereby all of the libc and libcpp imports live in one place: For example

from libcpp cimport vector
from libcpp cimport ...

Describe the solution you'd like

As more modules are Cythonised as part of CZIEOSS4 Performance track changes, we should continue to add them to the public Cython API.

This however does require some decisions as to what is public from the Cython side and what should remain internal.

Alternately if people do not like this idea we should probably discuss it and now is likely a good time. :)

Describe alternatives you've considered

Have everything be private and internal to MDAnalysis.

hmacdope avatar Jun 25 '22 04:06 hmacdope

  • Interoperability is good.
  • Given that there's an established pattern for this approach, libmda sounds sensible.
  • However, I feel we should be more explicit and name it libmdanalysis so that it is especially clear in other code which API is being used. (We use "mda" a lot internally but using it on external-facing code is not good branding — search for "mda" and see what you find, it ain't us ;-) )
  • In terms of what should go in there: presumably whatever is stable and can be sensibly used from the outside. What do you have in mind? Timestep, AtomGroup, Atom, Universe are really our core data structures. Which of these will be cythonized?

orbeckst avatar Jun 28 '22 00:06 orbeckst

Thanks @orbeckst! I will open ~an issue~ a PR to fix the naming.

The things that could be exposed as you say have to be sensible and stable.

  • Timestep in its entirety
  • Atomgroup (this is somewhat complicated by the amount of metaclass-y stuff going on eg Transplants)
  • A fast topology lookup table
  • Universe (to be investigated)
  • Others TBA

This is just a rough first idea and is obviously will be subject to a bit of evolution as we go.

hmacdope avatar Jun 28 '22 02:06 hmacdope

@MDAnalysis/coredevs with this in mind would people be open to exposing libdcd and libmdaxdr as part of our public facing cython library Would be a simple change but may advertise the libdcd and libmdaxdr are capable of being used standalone.

hmacdope avatar Nov 04 '22 05:11 hmacdope

Yes, why not?

Do you envisage packaging them separately?

orbeckst avatar Nov 04 '22 18:11 orbeckst

So something that came up in a conversation somewhere else - libmdanalysis is a suitably big name clash with lib that it has a high potential to make folks confused. Is there any way we can avoid this direct clash? Could we call it anything else or could we put libmdanalysis within lib at least?

IAlibay avatar Nov 04 '22 18:11 IAlibay

We can move it within lib for sure. The aim was just one entry point for the cython if you want it so it doesn't really matter where it goes

hmacdope avatar Nov 04 '22 22:11 hmacdope

We can move it within lib for sure. The aim was just one entry point for the cython if you want it so it doesn't really matter where it goes

Is there a potential issue with cyclic imports?

IAlibay avatar Nov 04 '22 22:11 IAlibay

Possibly I would have to test. It’s only for .pxd cimports which act more like C headers so I’m not sure if cyclical will be a problem.

The upside is that you can do stuff like

‘’’ from MDA.lib.libmdanalysis cimport timestep’’’

But the possibly confusing part is that the headers are elsewhere.

On Sat, 5 Nov 2022 at 9:08 am, Irfan Alibay @.***> wrote:

We can move it within lib for sure. The aim was just one entry point for the cython if you want it so it doesn't really matter where it goes

Is there a potential issue with cyclic imports?

— Reply to this email directly, view it on GitHub https://github.com/MDAnalysis/mdanalysis/issues/3734#issuecomment-1304297997, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3RHCZDOJDIJDAGX35CDXTWGWCNDANCNFSM5ZZVE3MA . You are receiving this because you were assigned.Message ID: @.***>

-- Hugo MacDermott-Opeskin

hmacdope avatar Nov 04 '22 22:11 hmacdope

Are people happy with the name libmdanaylsis moved inside lib or is this still too confusing. Any other possible name suggestions?

hmacdope avatar Nov 06 '22 10:11 hmacdope

I’m fine with the name, just spelled correctly ;-).

orbeckst avatar Nov 06 '22 16:11 orbeckst

I'm happy with lib.libmdanalysis if that makes sense to folks / doesn't cause weird import issues. I take view that folks that will use libmdanalysis will understand the codebase more than the newcomers who might get confused by the presence of two lib* directories.

IAlibay avatar Nov 06 '22 16:11 IAlibay

Now that docs have been added as of #3921 we can add the libdcd and libmdaxdr cython headers to libmdanalysis as part of #3888 and #3892 respectively.

hmacdope avatar Nov 21 '22 11:11 hmacdope

I think we can close this now with the note that folks add their public facing cython to libmdanalysis from now on.

hmacdope avatar Jan 20 '23 09:01 hmacdope