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

workflow patches #451

Merged
merged 25 commits into from
Jul 11, 2024
Merged

workflow patches #451

merged 25 commits into from
Jul 11, 2024

Conversation

jmbuhr
Copy link
Collaborator

@jmbuhr jmbuhr commented Jun 28, 2024

A collection of fixes as we explore various ways of interacting with kimmdy both for users and plugin developers.

  • Tasks are now consistently called tasks to not confuse them with steps (as in RecipeSteps)
  • nrexcl of the reactive moleculetype is more exposed
  • tasks are now always set up on init of the RunManager and the logging is configured on init of the Config (because these things always have to happen there right after anyways). This makes it easier to step through kimmdy from a notebook.
  • more and earlier null checks
  • all relative paths in the config now make proper use of config.cwd
  • xtcs can be used with the single reaction analysis workflow
  • quarto version has been updated to make our docs look better on mobile

Up for discussion:

  • RunManager.crr_tasks have been renamed to slotted_tasks to highlight the role of slotting in tasks between the predetermined queue. Since it is now easier to run kimmdy interactively this property is more exposed and I wanted a proper word and less angry animal sounds. Still not entirely happy with the name, naming things is hard.

@jmbuhr jmbuhr changed the title [draft] workflow patches workflow patches Jul 5, 2024
@jmbuhr jmbuhr added testthis and removed testthis labels Jul 5, 2024
Copy link
Collaborator

@KRiedmiller KRiedmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Regarding the naming, maybe better prio_tasks? intermediate_tasks is a bit long

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Jul 9, 2024

Or maybe full circle back to current_tasks, just not abbreviated? Length is not really a factor with autocomplete these days, so I would always err on the side of readability.

@ehhartmann
Copy link
Collaborator

I like priority_tasks.

Copy link
Collaborator

@ehhartmann ehhartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many tests are failing for this branch?

@jmbuhr jmbuhr added testthis and removed testthis labels Jul 9, 2024
@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Jul 9, 2024

How many tests are failing for this branch?

None of the ones that we run in CI, see https://github.com/graeter-group/kimmdy/actions/runs/9809976871/job/27089067802

but the restarting is still broken. and test_grappa_partial_parameterization still fails.

@ehhartmann
Copy link
Collaborator

The charged peptide simulation doesn't fail? Did you pull grappa recently?

@jmbuhr
Copy link
Collaborator Author

jmbuhr commented Jul 9, 2024

It probably would, I'm not running the tests that require PLUMED, but that's entirely out of scope for this PR.

@jmbuhr jmbuhr merged commit b100117 into main Jul 11, 2024
1 check passed
@jmbuhr jmbuhr deleted the integration-patches branch July 11, 2024 10:05
jmbuhr added a commit that referenced this pull request Jul 11, 2024
changes from #451

fix: properly compare to empty dicts

refactor(config): rename recursive_dict argument to opts
  to reflect its function for a user, not the mode of operation

feat: add xtc option to config

refactor: format json

feat: always setup tasks on init of RunManager

feat: configure logger from init of Config
  (since this the only time we do it is after initializing the config,
  using the config)

refactor(runmanager): rename step to task_name for consistency

docs: update quarto version

fix: resolve all non-relative paths in config relative to config.cwd

refactor(runmanager): rename crr_tasks to priority_tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants