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

remove cython from setup.py #504

Merged
merged 3 commits into from Mar 17, 2021
Merged

remove cython from setup.py #504

merged 3 commits into from Mar 17, 2021

Conversation

mikemhenry
Copy link
Contributor

Description

Remove cython from setup.py

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelogNotable points that this PR has either accomplished or will accomplish.

Status

  • Ready to go

@mikemhenry
Copy link
Contributor Author

@jaimergp anything else that should end up in this PR?

Regardless on what we decide on conda-forge/openmmtools-feedstock#6 we should make a 0.20.2 release that does things better!

@jaimergp
Copy link
Member

Now that you mention it, I'd like to remove the six dependency. We claim to support Pyrthon 3.6+ only, but https://github.com/choderalab/openmmtools/blob/master/openmmtools/sobol.py has this weird xrange py2k->py3k leftover. Removing that six import and replacing range with xrange would be nice to have here too.

@Lnaden
Copy link
Contributor

Lnaden commented Mar 17, 2021

six import and replacing range with xrange

Did you mean "replacing xrange with range" instead?

@jaimergp
Copy link
Member

six import and replacing range with xrange

Did you mean "replacing xrange with range" instead?

Oops, yes, exactly.

@mikemhenry
Copy link
Contributor Author

That sounds good, I don't want to explode this release too much since there is some python 2.7 cruft (see openmmtools/utils.py line 691) But I can remove the dependency and use range.

@Lnaden
Copy link
Contributor

Lnaden commented Mar 17, 2021

(see openmmtools/utils.py line 691)

That works for Py3 as well, so there isnt explicit harm in leaving it for now, but it probably could be cleaned up at a future time.

But I can remove the dependency and use range.

Yes to this. Anything which further and fully severs our ties to Python 2 the better, that is until we have to start using twelve.

@mikemhenry
Copy link
Contributor Author

Agreed! I'll add @jaimergp and @Lnaden as reviewers -- let me know if there are any more changes you think we should make.

Copy link
Contributor

@Lnaden Lnaden left a comment

Choose a reason for hiding this comment

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

LGTM. We'll keep this simple and move on.

@jaimergp
Copy link
Member

We should note that the last conda-forge release depends on ambertools, which is not available on Windows. I am not sure where this is used in the package, but maybe it should be reassessed? If we have find something, we need soft-imports and safeguards for missing executables.

There's also another xrange instance in openmmtools/scripts/test_openmm_platforms.py.

@Lnaden
Copy link
Contributor

Lnaden commented Mar 17, 2021

The ambertools stuff comes from the data dirs: https://github.com/choderalab/openmmtools/tree/483765d3b87395834745e79275469ce6c2c3fa13/openmmtools/data The shell scripts in there use tleap

@jaimergp
Copy link
Member

Those scripts are not run often, right?

@jaimergp
Copy link
Member

I am also seeing mdtraj and pandas required in testsystems, but they are try-except guarded. The OpenEye toolkit is also there, but not available on conda-forge.

@mikemhenry
Copy link
Contributor Author

There's also another xrange instance in openmmtools/scripts/test_openmm_platforms.py.

I don't see it? What line?

@mikemhenry
Copy link
Contributor Author

The OpenEye toolkit is also there, but not available on conda-forge.

I dug a bit and while that will throw an error -- it isn't clear to me how to trivially replace the functionality or disable the functionality (it is really limited in use and a quick grok shows that it is used in test systems) but I can throw a different error if we want other than the one openeye will throw when it says it is unlicensed.

Is this fix something you wanted in this patch or can we make this an issue for another day @jaimergp ?

@jaimergp
Copy link
Member

I don't see it? What line?

Ah, you got it already! Disregard then.

Is this fix something you wanted in this patch or can we make this an issue for another day?

Definitely a different issue.

@mikemhenry
Copy link
Contributor Author

Okay! I will merge this and cut a new release, thank you both for all of your help!

@mikemhenry
Copy link
Contributor Author

ALMOST FORGOT TO UPDATE THE CHANGELOG!

@mikemhenry mikemhenry enabled auto-merge (squash) March 17, 2021 15:07
@mikemhenry mikemhenry merged commit 3f2cfc0 into master Mar 17, 2021
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.

None yet

3 participants