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

IMDv3 Integration #4694

Closed
wants to merge 1 commit into from
Closed

IMDv3 Integration #4694

wants to merge 1 commit into from

Conversation

ljwoods2
Copy link
Contributor

@ljwoods2 ljwoods2 commented Aug 30, 2024

Draft PR for integrating the IMD (Interactive Molecular Dynamics) new v3 protocol into MDAnalysis.

The IMDReader hooks into the IMDClient API, which is comprised of 4 methods:

__init__(): Connect to the simulation engine and send it a go signal to start sending IMD data

get_imdsessioninfo(): Returns an `IMDSessionInfo` object which contains info on whether xvf/energies/dimensions are sent in the stream

get_imdframe(): Returns an `IMDFrame` or raises EOFError if the IMD stream is over

stop(): Cleanup method that ensures the `IMDClient` disconnects from the simulation server

There are a few different alternatives for how the IMDReader is integrated into MDAnalysis.

IMDReader in main codebase, IMDClient in a separate (non-MDAKit) repo

The first rule of MDAKits is don't talk about MDAKits. The second first rule is that an MDAKit has to actually use MDAnalysis, so if the IMDReader is in this codebase, IMDClient can't be an MDAKit.

The advantage of splitting up the classes like this is that:

  1. Users can use IMDv3 in MDAnalysis without installing additional packages
  2. IMDClient remains decoupled from MDAnalysis, meaning other groups can use the client to parse IMDv3 streams
  3. The IMDClient is free to do time-consuming and client-specific tasks like:
    • Test the IMDClient's ability to automatically pause a running simulation when its buffer fills, which is a complex multithreaded testcase that looks nothing like anything else in MDAnalysis. Here's some example tests
    • Build simulation engines that implement IMDv3 in the CI and test them against the client. These tests will potentially take several hours but add a lot of value. For example, LAMMPS offers several compiler options that impact IMD which are currently untested in the LAMMPS codebase. These options could be tested in the CI for different releases and on the develop branch. Example LAMMPS integration test class: base class, lammps class

This is the solution I'm most excited about and that this PR illustrates, but I can see that having the IMDReader and IMDClient in separate repos could create an integration challenge if the IMDClient needs to change significantly in ways that impact the API.

IMDReader & IMDClient in an MDAKit

This option still allows the client to be distributed separately from MDAnalysis and still allows more complex CI usage to stay out of the main codebase, but makes it so that users of MDA will have to install an additional package to use the IMDReader

IMDReader & IMDClient in MDAnalysis main codebase

I feel strongly that this is not a good idea. The IMDClient tests would add complexity and runtime to an already complex MDA CI/CD pipeline, add a (very annoying, multithreaded) maintenance burden to core MDA developers, makes IMDClient unusable by other groups, and makes it more difficult to make rapid future changes to the IMDClient (like potentially replacing the implementation of socket interaction codes with Cython while keeping the API the same).

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4694.org.readthedocs.build/en/4694/

@pep8speaks
Copy link

Hello @ljwoods2! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 6:79: W291 trailing whitespace
Line 7:80: E501 line too long (87 > 79 characters)
Line 18:80: E501 line too long (82 > 79 characters)
Line 117:80: E501 line too long (80 > 79 characters)
Line 120:80: E501 line too long (91 > 79 characters)
Line 148:80: E501 line too long (95 > 79 characters)
Line 156:80: E501 line too long (83 > 79 characters)
Line 193:9: E722 do not use bare 'except'
Line 218:80: E501 line too long (80 > 79 characters)
Line 235:80: E501 line too long (80 > 79 characters)
Line 280:80: E501 line too long (84 > 79 characters)

Line 85:80: E501 line too long (83 > 79 characters)
Line 166:80: E501 line too long (80 > 79 characters)
Line 307:80: E501 line too long (88 > 79 characters)
Line 318:80: E501 line too long (84 > 79 characters)
Line 349:80: E501 line too long (81 > 79 characters)
Line 361:80: E501 line too long (80 > 79 characters)
Line 412:80: E501 line too long (80 > 79 characters)
Line 508:80: E501 line too long (106 > 79 characters)

Copy link

Linter Bot Results:

Hi @ljwoods2! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/10628327807/job/29462958581


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@ljwoods2 ljwoods2 changed the title IMD Integration IMDv3 Integration Aug 30, 2024
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 76.57658% with 26 lines in your changes missing coverage. Please review.

Project coverage is 93.51%. Comparing base (d73995a) to head (32aa1f8).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
package/MDAnalysis/coordinates/IMD.py 76.14% 18 Missing and 8 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4694      +/-   ##
===========================================
- Coverage    93.62%   93.51%   -0.11%     
===========================================
  Files          173      186      +13     
  Lines        21419    22595    +1176     
  Branches      3978     3997      +19     
===========================================
+ Hits         20053    21130    +1077     
- Misses         903      994      +91     
- Partials       463      471       +8     

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

@ljwoods2 ljwoods2 marked this pull request as draft August 30, 2024 07:12
@ljwoods2
Copy link
Contributor Author

ljwoods2 commented Sep 3, 2024

Closing since it was decided that IMDReader & IMDClient will exist in an MDAKit

@ljwoods2 ljwoods2 closed this Sep 3, 2024
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