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

test installed module #376

Merged
merged 8 commits into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@loriab
Contributor

loriab commented Jun 22, 2018

in this PR

  • installs a few more files so that one can run pytest on an installation
  • allows some of the heavier (i.e., jupyter, matplotlib) and noncore (i.e., pubchempy) dependencies to be optional by moving the imports and adding proper pytest skipping
  • tested on py27, 35, 36
  • fyi, the setup.py/MANIFEST.in handling of examples/ works just fine for python setup.py install (that is, places the examples dir w/i the module). however, if you python setup.py install --single-version-externally-managed, that will place the examples dir into PREFIX/openfermion/.

Q

  • there's some repeated code (_module_import()). if you decide you like this PR, could you suggest a good shared access place for it?
  • despite making some dependencies optional, I didn't interfere with requirements.txt, as I can control them independently through conda. but if you like this change, should be tweaked.

longer term Q

@googlebot

This comment has been minimized.

googlebot commented Jun 22, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no label Jun 22, 2018

@loriab

This comment has been minimized.

Contributor

loriab commented Jun 22, 2018

I signed it!

@googlebot

This comment has been minimized.

googlebot commented Jun 22, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jun 22, 2018

@loriab

This comment has been minimized.

Contributor

loriab commented Jun 22, 2018

Sorry, I didn't realize I was springing a PR on you at the end of a release cycle yesterday.

Looking at the coverage fail, I think a decision on optional deps and requirements.txt is needed before I could tweak one of the test matrix into covering the extra lines.

@googlebot

This comment has been minimized.

googlebot commented Jun 25, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 25, 2018

@jarrodmcc

This comment has been minimized.

Collaborator

jarrodmcc commented Jun 25, 2018

Thanks for the contribution Lori, sorry we haven't gotten to it yet, as you saw a bit of a release rush last week. We'll discuss some of the questions you raised tomorrow and get back to you, but I was wondering in the meantime if you could describe in broader terms what you're working towards. Is it mostly enabling the conda installation of OpenFermion-Psi4 or do you have a larger plan as well for some of these changes?

The reason OpenFermion-Psi4 doesn't have many tests at the moment is simply that there were a few more hurdles in getting automatic testing setup on Travis. That doesn't entirely excuse the lack of offline tests that can be run, but it's a todo item that we haven't quite gotten to yet for various reasons.

We look forward to helping enable further functionality between Psi4 and OpenFermion though, we're very excited to have Psi4 developers such as yourself interested in OpenFermion!

@loriab

This comment has been minimized.

Contributor

loriab commented Jun 25, 2018

Thanks for taking a look, @jarrodmcc. Didn't mean to make a surprise PR at the end of a release cycle.

My goals are modest — I just wanted to check that Psi4 is still working for OpenFermion (we're not great at API/ABI stability and some changes to pybind11 have required rebuilding of downstream projects). Beyond that, if there was an easy way to monitor ongoing compatibility, rather than annual testing, I'd like to set it up. Beyond that, since conda packages are free after the real work of devising tests, if you wanted conda packages built and/or distributed in the psi4-rt metapackage (collects optional run-time dependencies like Grimme's dftd3 and psi4 plugins`), glad to do so.

As far as monitoring ongoing compatibility, on the psi side, I'd like a little smoke test case for here so that if openfermion/openfermion-psi4 are present, it'll check that psi4 is still working. On the openfermion-psi4 side, I can well understand the travis hurdles. Something like https://github.com/cdsgroup/resp/blob/master/.travis.yml may be helpful in avoiding a big compilation.

My focus on installed, as opposed to source-dir, testing is the result of trying to test multiple projects and also b/c conda packaging encourages it.

Btw, I'm always glad to rebase when you need it. That may help avoid CLA troubles.

@jarrodmcc

Thanks again for this PR! Don't worry about the CLA bot, I'll take care of it once the PR is approved, it has a tendency to complain whenever we use the Github merge.

In response to your specific questions...

Which requirements did you imagine changing to optional based on the current changes?

As for conda packages, I think for now we are happy just maintaining the PyPI package, as installation in anaconda using it seems to work reasonably seamlessly as well. We are happy for you to have a separate conda build that you make sure runs with Psi4 as well in your own channel. Would it be helpful for us to include such a conda build file for optional use in this repo? As for versions, I would recommend pinning it to release versions of 0.7 or greater rather than the Github master at the moment. This will probably keep the interface more stable, though like yourselves we tend to move things around a bit still.

We could work to add a few more tests before your next release to the OpenFermion-Psi4 plugin, do you have an expected time frame for that?

from openfermion.config import THIS_DIRECTORY
def _module_import(plug):

This comment has been minimized.

@jarrodmcc

jarrodmcc Jun 26, 2018

Collaborator

As you asked about in your PR, I think we can put this _module_import helper function into utils/testing_utils.py

This comment has been minimized.

@jarrodmcc

jarrodmcc Jun 26, 2018

Collaborator

Also, for possible future users of this function, can you add a docstring and specification for the plug parameter?

else:
return True
using_nbtools = pytest.mark.skipif(not (_module_import('nbconvert') and _module_import('ipykernel')),

This comment has been minimized.

@jarrodmcc

jarrodmcc Jun 26, 2018

Collaborator

Minor style comment, but make sure your files pass pep8 checking (line length in the case of the lines below for the extended strings). It's mostly manual checking at the moment, but we will be converting to enforced pylinter checking soon for PR's

@loriab

This comment has been minimized.

Contributor

loriab commented Jun 26, 2018

Thanks for the review. I think that commit patches up the specific code issues.

Which requirements did you imagine changing to optional based on the current changes?

I think jupyter and matplotlib are clear candidates for optional deps, as they're only used for the tests. I've also set up pubchempy as optional since it's neither part of the core scientific python stack nor core openfermion functionality. But I can see how you'd be more hesitant to classify it optional since the module itself imports it.

As for conda packages, I think for now we are happy just maintaining the PyPI package, as installation in anaconda using it seems to work reasonably seamlessly as well. We are happy for you to have a separate conda build that you make sure runs with Psi4 as well in your own channel. Would it be helpful for us to include such a conda build file for optional use in this repo?

Sounds good to me, thanks. No need to include a conda recipe file here (unless you'd like to) as I keep all recipes in a separate repo.

As for versions, I would recommend pinning it to release versions of 0.7 or greater rather than the Github master at the moment. This will probably keep the interface more stable, though like yourselves we tend to move things around a bit still.

Will do. Last week's release actually solves this problem nicely.

We could work to add a few more tests before your next release to the OpenFermion-Psi4 plugin, do you have an expected time frame for that?

Next release was due last month :-) I'm finally about to cut 1.2rc3. So 1.2 will probably not wait on openfermion integration tests. This is part of a general effort (before my ecosystem diagram gets completely unmanagable) to set up testing interlocks so Psi4 can refactor freely.

@jarrodmcc

This comment has been minimized.

Collaborator

jarrodmcc commented Jun 26, 2018

Thanks for the changes! I'm going to go ahead and merge this in spite of coveralls hanging since the tests are passing and it looks good.

@jarrodmcc jarrodmcc merged commit 7734093 into quantumlib:master Jun 26, 2018

1 of 2 checks passed

cla/google CLAs are signed, but unable to verify author consent
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@loriab loriab deleted the loriab:testtweaks branch Jun 26, 2018

@babbush

This comment has been minimized.

Contributor

babbush commented Jun 27, 2018

@loriab I want to reiterate how much the OpenFermion developers appreciate consideration from the folks at Psi4! Thanks again!

Additionally, it is our policy that anybody with merged PRs on OpenFermion is entitled to be listed as an author in the README / NOTICE files and also on the OpenFermion release paper (https://arxiv.org/abs/1710.07629), which we keep on arXiv so that we can update whenever we want.

If you'd like to be listed as an author on the README / NOTICE, consider opening a PR to add yourself in the alphabetical section. If you'd like to be on the release paper, please email myself or Jarrod or so that we can send you an overleaf link to the paper draft.

gaming-hacker added a commit to gaming-hacker/OpenFermion that referenced this pull request Jul 18, 2018

Merge branch 'master' into v1
* master:
  version bump (quantumlib#399)
  Fix version-incompatible string checks using six (quantumlib#397)
  Added Haar random function (quantumlib#398)
  remove recommendation (quantumlib#394)
  Move up plugins (quantumlib#393)
  sort terms for string output (quantumlib#390)
  fix RNG seeding in testing functions (quantumlib#389)
  Updated logo (quantumlib#388)
  Second logo (quantumlib#387)
  explicitly removed handling of complex eri matrices (quantumlib#386)
  fixed poor test (quantumlib#383)
  Low rank options (quantumlib#381)
  fixing zero constant error (quantumlib#380)
  Add page in docs for collecting projects and papers (quantumlib#379)
  Low rank circuits (quantumlib#378)
  Allow taking expectation of LinearOperator and switch HF state to numpy array (quantumlib#377)
  Low rank (quantumlib#373)
  Test installed module (quantumlib#376)

# Conflicts:
#	src/openfermion/utils/__init__.py
#	src/openfermion/utils/_low_rank.py
#	src/openfermion/utils/_low_rank_test.py
#	src/openfermion/utils/_pubchem_test.py
#	src/openfermion/utils/_testing_utils.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment