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

Too Much Coupling to SOMD #11

Open
1 of 5 tasks
fjclark opened this issue Nov 19, 2024 · 3 comments
Open
1 of 5 tasks

Too Much Coupling to SOMD #11

fjclark opened this issue Nov 19, 2024 · 3 comments
Assignees

Comments

@fjclark
Copy link
Collaborator

fjclark commented Nov 19, 2024

A3FE is currently too coupled to SOMD. In preparation for extending support to GROMACS, we should reduce this coupling. Specifically, we should:

  • 1.Remove the run_somd.sh file, from which the SLURM options are read. Replace with a Pydantic SlurmConfig class which can be configured in the code, or read from a yaml/json file if not supplied.
  • 2. Remove the template_config.cfg. As above, replace with a Pydantic SomdConfig class (which should be derived from a general EngineConfig class which can be configured in the code, or else read from a yaml/json file if not supplied
  • 3. Try and move any engine-specific logic out of the run classes. Currently, a few are contaminated by SOMD-specific logic
  • 4. Update defaults to be engine-specific. Probably do this by having dictionaries of defaults with engines as keys. Store the engine as a property of the simulation runner. This will allow easier GROMACS integration.
  • 5. Add pre-commit hooks to enforce formatting etc.
@fjclark
Copy link
Collaborator Author

fjclark commented Nov 19, 2024

When you have time, could you please have a shot at 1. @Roy-Haolin-Du?

I'd suggest (but feel free to ignore) that you:

  • Update your fork to include the feature-decouple-somd branch (I will create this once we've merged in the feature-charged-ligands PR is merged). Develop on the feature-decoupled-somd branch of your fork and create a PR to the same brach in this repo when your code is ready.
  • Take a look at all the places run_somd.sh appears in the code, and think about how the new SlurmConfig class will slot in instead. For example, we'll be able to delete a lot of checks that run_somd.sh exists/ copying around of this file. It can helpful to chat to GitHub copilot about the code (try using e.g. "@ workspace"). I'm currently trying Cursor (https://www.cursor.com/) which seems to be slightly better (and is very easy to migrate to from VSCode, as it's a fork).
  • Start by creating a configuration directory at the same level as run, read etc. All the Pydantic configuration classes will live here. Create configuration/system_preparation.py file and move the SystemPreparationConfig from run/system_prep to here. Make any required adjustments to the wider codebase (e.g. you'll have to make sure it's imported from its new location), format with ruff (e.g. ruff format .), and make sure the tests run (pytest a3fe). Commit the changes.
  • Although at this stage we've hardly made any changes, open a PR from your feature-decouple-somd to the one in this repo. We won't merge till much later, but this will allow me to provide code review as we go along.
  • Next, create configuration/slurm.py , where SlurmConfig will live. Have a look at SystemPreparationConfig/ read the Pydantic docs to get an idea of how to write SlurmConfig. This should have a to_slurm_header method which writes out the slurm header, and a to_slurm_script(script_body) method where script body is the bit of code which gets written under the slurm header and after srun, e.g. somd-freenrg -C config.cfg. Make sure that SlurmConfig can be serialised to/ read from json, or better, toml or yaml.
  • Write tests to make sure SlurmConfig works as you expect.
  • Remove run-somd.sh from everywhere in the code and figure out how to pass around SlurmConfig. This should likely be stored as an attribute of all SimulationRunners. This is probably the hardest bit. Avoid writing out scipts as much as possible - the only place we should write out the slurm script is likely during job submission. We want a "single source of truth" and don't want old scripts sitting around and conflicting with update SlurmConfigs.
  • Update the tests/ write more to make sure nothing is broken/ SlurmConfig works as expected with the wider code base.
  • Update the docs to show users how to use SlurmConfig and update the changelog under version 0.3.0 with the changes you've made.

The above plan will almost certainly not work as expected and will not be the optimal way of doing things - so please feel free to adapt, and discuss issues here or on the PR you will create.

Thanks!

@fjclark
Copy link
Collaborator Author

fjclark commented Nov 22, 2024

I've now created a feature-decouple-somd branch @Roy-Haolin-Du, so feel free to have a shot at the above when you get time. Thanks!

@fjclark
Copy link
Collaborator Author

fjclark commented Jan 7, 2025

Another thing we should do is to write out the a3fe version to all logs/ possibly store it as an attribute of all simulation runners. If an update breaks loading of an old pkl file (and we should add tests to check this more closely), then this would allow the user to select a working version of a3fe.

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

No branches or pull requests

2 participants