Geometry engine platform usage cleanup

[ijpulidos]

Description

These changes make the CPU platform to be the default platform for the geometry engine.

While working on these changes, it was noted that some of the utility functions lived in the wrong module under perses.tests.utils, they were moved to a "better" module in perses.dispersed.utils, since we should reserve the perses.tests.utils module for things that are only required in our tests.

Backward compatibility is achieved by having variables in perses.tests.utils to point to the new perses.dispersed.utils functions as needed. This is to avoid breaking workflows of users calling these functions directly from perses.tests.utils,

Motivation and context

It was noted in #1091 (comment) that we didn't want to change the default platform in general for that module, but just to change the platform used in the geometry engine.

How has this been tested?

Tested locally, verifying that the CPU platform is exclusively used when generating HTFs. This also gives better performance than using CUDA platform for "big" and "small" systems.

Change log

Should be the same as in #1091 adding the following

Importing utility functions from ``perses.tests.utils`` for production code is now deprecated and will not be a possibility in the upcoming 0.11 release.

[codecov]

Codecov Report

Merging #1092 (7154a6f) into main (c96e655) will increase coverage by 0.01%.
The diff coverage is 96.47%.

[jchodera]

Thanks! This looks like a significant improvement!

[zhang-ivy]

@ijpulidos : Everything looks good except I actually think we should keep check_system, compute_potential_components, generate_endpoint_thermodynamic_states, validate_endstate_energies and validate_endstate_energies_point in tests/utils.py, as all of these functions are used in tests, so I don't think its wrong to have them in tests/utils.py. Also, by nature, all of these functions exist so that we can run an energy validation check , so by nature, these can be considered used for testing -- its just that the energy validation check is needed (or is optional) in some places in the main API as well as in the tests.

While working on these changes, it was noted that some of the utility functions lived in the wrong module under perses.tests.utils, they were moved to a "better" module in perses.dispersed.utils, since we should reserve the perses.tests.utils module for things that are only required in our tests.

Is it considered a software best practice to only keep functions used solely in tests in test/utils.py? If a function is used in both a test and in the main API, I would assume its ok to keep it in test/utils.py

I also don't like that in the current PR, validate_endstate_energies_point is now split up from validate_endstate_energies_md -- these functions do the same thing, but one of them evaluates a point energy, and the other runs some MD before evaluating the energies.

[ijpulidos]

@zhang-ivy While this is mostly a convention and not a well-established "best practice", the rationale here is that we don't want production code to rely on tests. In terms of organization, I think it's just better if we agree that the tests directory should be only for testing purposes, such that we can go and change things in this directory without the fear of altering anything but the tests themselves (we do want tests to change, since these are not a "contract" with our users). In general I think it is just better to have less points in the code that can alter our production, as possible.

Just to be clear, I only moved out of tests/utils.py the functions that are used outside tests and affect our production code. This way we can be more careful about changing these in the future. This is also part of the reasons why I felt like changing the global DEFAULT_PLATFORM in #1091 was "safe", since I wasn't expecting things inside tests to change production.

I also think that separating the validate_endstate_energies_md is not ideal, I could put it wherever the other validate_endstate_energies* live, but maybe this is just showing us that we have redundant code and maybe having validate_endstate_energies_md might not be needed at all (just a guess, I haven't checked in detail). Since the validate_endstate_energies_md is only used in tests. Here I'm just trying to minimize the need for changes while maintaining a "correct" structure.

[zhang-ivy]

While this is mostly a convention and not a well-established "best practice", the rationale here is that we don't want production code to rely on tests. In terms of organization, I think it's just better if we agree that the tests directory should be only for testing purposes, such that we can go and change things in this directory without the fear of altering anything but the tests themselves (we do want tests to change, since these are not a "contract" with our users). In general I think it is just better to have less points in the code that can alter our production, as possible.

Just to be clear, I only moved out of tests/utils.py the functions that are used outside tests and affect our production code. This way we can be more careful about changing these in the future.

Yes I understand all of your rationale for why you made the changes, but I still don't 100% agree with it, because I don't think we should be assuming that changing things in the test code won't affect production code and vice versa. Even if we do merge this PR as is, any time we change a function, we should search the repo for all occurrences to check how the change will affect subsequent calls to the function. Happy to be overruled here, though, as long as we make sure to search for the repo for all occurrences of the changed function in the future.

I also think that separating the validate_endstate_energies_md is not ideal, I could put it wherever the other validate_endstate_energies* live, but maybe this is just showing us that we have redundant code and maybe having validate_endstate_energies_md might not be needed at all (just a guess, I haven't checked in detail). Since the validate_endstate_energies_md is only used in tests. Here I'm just trying to minimize the need for changes while maintaining a "correct" structure.

validate_endstate_energies_md is needed because its more rigorous than validate_endstate_energies_point -- i.e. running MD and checking the energies for many snapshots is more rigorous than checking the energy for one snapshot. However, checking the point energies is faster, so I've written the function to allow us to keep the CI running relatively quickly.

[zhang-ivy]

Also, if we are going to keep the PR as is, I think we do need a changelog entry for this PR -- something that mentions that we are fixing a problem that was introduced in #1091 and that we moved a bunch of energy validation related functions used in both tests and production from tests/utils.py to dispersed/utils.py

[ijpulidos]

Happy to be overruled here, though, as long as we make sure to search for the repo for all occurrences of the changed function in the future.

Yes of course, we always need to check for all the occurrences, that was never in discussion and that's always been done. Sorry if maybe my wording implied otherwise, but that wasn't the intention. I'm just saying it is a fact that these things happen regardless if we check or not, because there are many unexpected consequences. And once this happens it is better to see if it happened inside tests (or not) and decide the priority of fixing these based on how they affect production or just tests. It just helps development when production is affected from less points. If you think of it as a directed graph, we just removed one edge in this graph and making it a bit less complex by not having our production depend on the tests/utils.py node (even though nothing is for free, of course).

validate_endstate_energies_md is needed because its more rigorous than validate_endstate_energies_point -- i.e. running MD and checking the energies for many snapshots is more rigorous than checking the energy for one snapshot. However, checking the point energies is faster, so I've written the function to allow us to keep the CI running relatively quickly.

The point here is that validate_endstate_energies_md is only being used in tests and it doesn't affect production. Maybe you (or other users) are using it for validating or processing results? If that's the case, that would be helpful to know and maybe we can find a better place for it. The structure I propose here is flexible and if we think we could use a better structure, such as having a separate module for validating energies, I don't know, say validate_energies.py or something like that, that's also a possibility.

Also, if we are going to keep the PR as is, I think we do need a changelog entry for this PR -- something that mentions that we are fixing a problem that was introduced in #1091 and that we moved a bunch of energy validation related functions used in both tests and production from tests/utils.py to dispersed/utils.py

Yes, we can do this, even though our rationale about the changelog is in terms of what affect users, as far as I can see. So, it is more about the "what" than the "how", as in, users don't really care that these functions now live somewhere else if they can still import them from perses.tests.utils. In the same way, I don't think users need to know this fixes something from other PR when the latter never made it into a release, we instead link to both PRs in the same changelog entry, and interested users could just check the PRs themselves. Avoiding cluttering the changelog. But we can mention this of course, if we think it is worth mentioning.

[mikemhenry]

I don't think we should be assuming that changing things in the test code won't affect production code and vice versa.

I think this should be a safe assumption, at least in the direction that changing testing code shouldn't break our production code.

Few notes, first, not every package even ships the testing code since it tends to be a waste of space since most end users don't run-or even know how to run-the tests that ship with the package. So when packages are arranged like:

my_package/
 foo.py
 bar.py
 __init__.py
tests/
 test_foo.py
 test_bar.py

The testing code doesn't get packaged for end users, so you can't rely on code in test_foo.py to make foo.py work.

Second, with well written pytest code, it can be hard to use the testing code anyway, since functions may be using fixtures which won't work. For example, this bit of testing code https://github.com/OpenFreeEnergy/gufe/blob/17357f9c23b6f6e242f7d4be667705315dd85669/gufe/tests/test_protein_molecule.py#L24-L29 has an argument PDB_181L_path which isn't defined in the file but due to how pytest + conftest.py works, the function works fine for testing code but wouldn't work elsewhere.

Third, it is confusing to end users and developers to be importing something from package_name.tests namespace since it is really antipattern. I'm not sure of any packages that do this, other than something that might be used in a tutorial.

[mikemhenry]

This is to avoid breaking workflows of users calling these functions directly from perses.tests.utils

I vote we break things. Do user scripts use these functions, or are they only used internally. If it is only internal, then we should break backwards compatibility. If users are using them, it is probably only @dominicrufa and @zhang-ivy (pharma friends are using the cli) so I rather we make breaking changes that are easy to fix (just different namespace) then having to keep around crud.

[zhang-ivy]

Yes of course, we always need to check for all the occurrences, that was never in discussion and that's always been done.

Maybe I missed something, but I assume this wasn't done with PR #1091 -- this is how I found the problem in the first place. I saw that DEFAULT_PLATFORM was changed, and then noted that its used in compute_potential_components and validate_endstate_energies, both of which are used in production pipelines and testing, which is why it was problematic to directly change DEFAULT_PLATFORM

The point here is that validate_endstate_energies_md is only being used in tests and it doesn't affect production.

Yes that's correct. It's only used in testing. I thought you had been saying it isn't needed at all, which is why I was explaining why it was needed. If we are sticking with Iván proposed structure, we should just leave validate_endstate_energies_md in tests/utils.py.

we instead link to both PRs in the same changelog entry, and interested users could just check the PRs themselves.

Ok, that's fine with me.

Few notes, first, not every package even ships the testing code since it tends to be a waste of space since most end users don't run-or even know how to run-the tests that ship with the package.

Yes, that's true, but for perses, the test code does get shipped with the rest of the code, so that's not a problem here.

I vote we break things. Do user scripts use these functions, or are they only used internally. If it is only internal, then we should break backwards compatibility. If users are using them, it is probably only @dominicrufa and @zhang-ivy (pharma friends are using the cli) so I rather we make breaking changes that are easy to fix (just different namespace) then having to keep around crud.

I think this comment highlights the reason why we are disagreeing. I would prefer to see the fewest changes to the code as possible, so that my current workflow/scripts are not disrupted. From a scientist's point of view, moving the functions from tests/utils.py to dispersed/utils.py doesn't change anything besides break my workflows, so I was trying to avoid the inconvenience, though the inconvenience is minor. From a software perspective, moving the functions has some benefits (which Iván and Mike discuss at length above), but I wouldn't say that the code is "crud" -- it works fine the way it is, but maybe is not completely ideal. Since the fix for workflow disruptions is very simple (changing the import and remembering that validate_endstate_energies_point should be imported differently than validate_endstate_energies_md), I think its ok to proceed with this PR (after removing backwards compatibility, as Mike suggests), but I think its important to point out the differences in our priorities as we need to consider both with every PR.

[ijpulidos]

I guess a way to move forward here is to keep the backward compatibility at least for the 0.10.x branch, such that users can still import from perses.tests.utils using this branch and release. And later remove it with the 0.11 release. We will let users know that we are deprecating this behavior and removing it in 0.11 through the release notes. Does that sound okay?

[mikemhenry]

but I wouldn't say that the code is "crud" -- it works fine the way it is, but maybe is not completely ideal

Sorry! I really didn't mean to call the code "crud" I meant to say that I consider keeping something to maintain backwards compatibility around "crud' since it is easy to loose track of things like that and makes the code harder to maintain.

I'm fine for keeping it backwards compatible in 10.x and removing the backwards compatibility in 11.x

I think its important to point out the differences in our priorities as we need to consider both with every PR.

I'm not sure how I would order these priorities, but I would consider these to be the main priorities/areas of concern in scientific software development to be:

• Produces correct science
• Reproducible
• Maintainable
• Don't mess up user stuff (unless there is a major version bump when using semver)

Moving the function breaks user stuff (boo) but makes the code easier to maintain (yay). I think we are in a good spot with moving the function, but doing it in a backwards compatible way.

I didn't do a good enough job reviewing #1091 as I took for granted that the location of the function was meaningful. A quick grep in the whole code base would have highlighted this issue, but instead I searched for where the DEFAULT_PLATFORM variable was used in the code base, and it was only in that module, so I didn't think to then search if other modules imported functions that used that variable.

[zhang-ivy]

I think the order of the bottom 2 bullets are where things get a bit grey depending on the case! But otherwise I agree

[mikemhenry]

I think we should move these to the top for import

[mikemhenry]

Move to the top, I know sometimes it is nice to keep imports local (optional dep, import side effects, slow import) but in this case I don't think it is needed.

[ijpulidos]

Why is it that you think so? dispersed/utils.py does import a LOT of things (even scipy), I think it is worth it to keep this in the lower level. Maybe what we can do to make the readability a bit better is to have them in the top of the method (_logp_propose in this case). Does that sound okay?

[mikemhenry]

maybe we make a new utils file here:
https://github.com/choderalab/perses/tree/main/perses/utils
that is like hybrid_topology_utils or something
I figure if we are going to move this, lets move it to the right spot

[zhang-ivy]

Yeah I agree dispersed/utils.py might not be the best place. I'm not actually sure what functionality dispersed/ is meant to contain.

I agree that perses/utils/ is a better place, maybe call it energy_validation.py instead of hybrid_topology.py, as the functions are used for generating htfs as well as geometry proposals.

[ijpulidos]

Does this sound reasonable? #1092 (comment)

[mikemhenry]

Move these up to the top as well

[mikemhenry]

Move to top

[ijpulidos]

Ok, from internal discussions with @mikemhenry and @zhang-ivy, separately, I think we might have reached an agreement for this PR and future changes.

0.11 release is going to be an API changing and breaking release. Judging from how these utility functions are being used, we probably want some of these (especially the validate_endstate_energies* ones) to be part of their respective HybridTopologyFactory objects, and probably have a factory pattern for these as well. There's actually an issue about this already opened in #1051 .

For the other functions (namely compute_potential_components), we need to evaluate what's the best location to put them, but since we are doing the current changes in a backward compatible way, I think it should be okay that we have them temporarily living in perses.dispersed.utils and users being able to import them from perses.tests.utils, for now.

Does that sound reasonable as a way to move forward with this PR? Thank you all for the feedback and comments, it's been really helpful.