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

Check availability of theseus/mmligner executables #99

Merged
merged 13 commits into from
Sep 13, 2021

Conversation

dominiquesydow
Copy link
Contributor

@dominiquesydow dominiquesydow commented Sep 10, 2021

Description

We don't have Windows versions for theseus and mmligner as of now, thus currently no working Windows opencadd version.

Our workaround suggested by @jaimergp:
Add safety checks for these packages (incl. informative text for our users). With such a workaround we will be able to offer this package for Windows, too (without functionality for the bits that need theseus and mmligner though).
If these packages will be available in the future, the respective bits in opencadd will work out-of-the-box, too.

More background details:

  • Our package is labeled as noarch, which means we actually build it on linux (could be any other platform, but condaforge chose that one) and then hope it works in other OSs as well
  • To be noarch our package needs to be pure python, no compiled extensions, and no per-platform dependencies
  • We do need per-platform dependencies because some of those used in opencadd are not on windows
  • We are ignoring that and adding checks in our code as a workaround
  • When the dependencies become available, it will work magically in opencadd

Todos

  • Add .safety_checks() method to BaseAligner but implement code in respective classes, see below
  • Add _safety_checks() implementation to TheseusAligner and MMLignerAligner:
    from distutils.spawn import find_executable
    
    def align_with_theseus():
       theseus = find_executable("theseus")
       is theseus is None:
           raise Error("theseus cannot be located. Is it installed?")
       # proceed normally 
  • Add _safety_checks() to [MDAnalysisAligner]; only added for consistency across engines; simply passes (no executable check necessary since conda package available for all OS)
  • Call these methods in BaseAligner.calculate method

Questions

None.

Status

  • Ready to go

@dominiquesydow dominiquesydow self-assigned this Sep 10, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2021

Codecov Report

Merging #99 (d18ebdd) into master (88654c6) will decrease coverage by 0.35%.
The diff coverage is 82.35%.

@dominiquesydow dominiquesydow merged commit 47b71ca into master Sep 13, 2021
@dominiquesydow dominiquesydow deleted the theseus-mmligner branch September 13, 2021 10:58
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