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

Migrate common settings to omm_utils #512

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Migrate common settings to omm_utils #512

merged 3 commits into from
Aug 2, 2023

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Aug 2, 2023

This doesn't really do much but both @hannahbaumann and I are working on branches that rely on this based on some initial design on what's reusable in #237.

Since things are moving on the settings side of things, I thought it'd be best to just do this in a separate commit so that it's easier to deal with merge conflicts later.

Developers certificate of origin

@IAlibay
Copy link
Member Author

IAlibay commented Aug 2, 2023

Huh I did not expect this - @dwhswenson is there a reason the key should change if the import location changes?

@dwhswenson
Copy link
Member

is there a reason the key should change if the import location changes?

Yep. Key for a Protocol comes from the hash of the JSON dump of the serialization of settings. In order to deserialize settings, the serialized form needs to know where we import the settings from. So moving things around is expected to break keys. Aliasing (via imports) will allow backward compatibility, but will still lead to a different key.

@IAlibay
Copy link
Member Author

IAlibay commented Aug 2, 2023

is there a reason the key should change if the import location changes?

Yep. Key for a Protocol comes from the hash of the JSON dump of the serialization of settings. In order to deserialize settings, the serialized form needs to know where we import the settings from. So moving things around is expected to break keys. Aliasing (via imports) will allow backward compatibility, but will still lead to a different key.

Do we think this is a blocker or as long as the settings themselves don't change we're still backwards compatible on deserialisation?

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 84.39% and no project coverage change.

Comparison is base (e727936) 91.81% compared to head (a47879f) 91.82%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #512   +/-   ##
=======================================
  Coverage   91.81%   91.82%           
=======================================
  Files         110      111    +1     
  Lines        6681     6689    +8     
=======================================
+ Hits         6134     6142    +8     
  Misses        547      547           
Files Changed Coverage Δ
openfe/protocols/openmm_utils/omm_settings.py 84.21% <84.21%> (ø)
openfe/protocols/openmm_rfe/equil_rfe_settings.py 100.00% <100.00%> (+12.55%) ⬆️
openfe/tests/protocols/test_rfe_tokenization.py 97.14% <100.00%> (ø)

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

@IAlibay IAlibay merged commit ab68095 into main Aug 2, 2023
6 of 7 checks passed
@IAlibay IAlibay deleted the move-settings branch August 2, 2023 16:13
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.

3 participants