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

Poetry and workflow changes #29

Merged
merged 19 commits into from
Jul 6, 2023
Merged

Poetry and workflow changes #29

merged 19 commits into from
Jul 6, 2023

Conversation

TL231
Copy link
Contributor

@TL231 TL231 commented Jun 22, 2023

Main changes:

Replace setup.py with poetry and added workflows based on qibo and qibosoq workflows as mentioned previously (#23, #22).
Additionally, resolved 2 dependency issue while fiddling with poetry:

  • Psi4 v1.8.0 hits me with a import error: undefined symbol: dkh_main_mp_dkh_ on windows 10 WSL, downgrading to v1.7.0 solves this issue so I added that to the dependencies.
  • openfermion v1.3 uses a deprecated SingleQubitGate function from cirq, this was resolved in version 1.5.0 which I updated the dependencies to.

TODO:

Missing Psi4 dependencies from poetry messes up the automated workflow, #26
holding this off for another branch since the issue is getting in the way of the other changes

@TL231 TL231 requested a review from chmwzc June 22, 2023 08:16
Copy link
Contributor

@chmwzc chmwzc left a comment

Choose a reason for hiding this comment

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

Thanks! Just 2 questions/issues:

  1. Just to confirm, replacing setup.py with poetry doesn't affect the current installation instructions (below) right?
git clone https://github.com/qiboteam/qibochem.git
cd qibochem
pip install .
  1. timer.dat is a temporary file from running PSI4, probably created while testing manually? Anyway, it shouldn't be included.

Copy link
Contributor

@chmwzc chmwzc left a comment

Choose a reason for hiding this comment

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

Thanks! I only have some minor changes to .gitignore.

Other than that, I haven't tried your new installation instructions with poetry yet, but just a thought: the main Qibo repository uses poetry and doesn't need it to install, so we might not need it here? I duno, might look into testing it, after this is merged?

@TL231
Copy link
Contributor Author

TL231 commented Jul 4, 2023

Thanks! I only have some minor changes to .gitignore.

Other than that, I haven't tried your new installation instructions with poetry yet, but just a thought: the main Qibo repository uses poetry and doesn't need it to install, so we might not need it here? I duno, might look into testing it, after this is merged?

I've checked the qibo docs and it says you can just "pip install ." the repo, I've tested it and it works so let me just revert the readme.
Still working on psi4 installation issue, psi4 does not offer a pip install, only a conda one, so I can't seem to add it into the dependencies. This means the automated testing and linting in the reused workflows (https://github.com/qiboteam/workflows) get an import error and stops, I have 2 solutions in mind to try, one is to see if adding conda install before running the reused workflow messes things up but that could cause it's own set of errors, the other is to make our own, but that kind of goes against the point of a reusable workflow.
Once I solved this issue, then it's time to merge into the main branch.

@damarkian
Copy link
Contributor

I'm writing the sphinx documentation for qibochem, and following a similar format with qibocal. Qibocal documentation mentions that it is essential to install qibolab as both work together.

So I'm thinking - will it be easier if we include the psi4 installation separately, as it is part of the conda package management instead of poetry? And I document this in the sphinx doc in a similar way with qibocal?

Copy link
Contributor

@chmwzc chmwzc left a comment

Choose a reason for hiding this comment

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

I'm thinking that maybe we should just take out the PSI4 tests for now, and merge this first, then leave the PSI4 CI tests to a separate PR? I believe that's what qibotn did when they had a similar issue running CI there.

environment.yml Outdated
@@ -12,5 +12,5 @@ dependencies:
- pip
- pip:
- openfermion>=1.5
- qibo
- pyscf
- qibo=0.1.10
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the restriction on the Qibo version?

Copy link
Contributor

@damarkian damarkian Jul 5, 2023

Choose a reason for hiding this comment

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

I agree with @chmwzc let's do what qibotn team did. Also, restriction on Qibo version above has to be >= or ==. But if there is no need for a restriction, it should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should've added a > there, just adding it as a reminder of the lowest working version.
Anyway, I agree with you since this dependency issue is out of the scope of the branch.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@fd87a17). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #29   +/-   ##
=======================================
  Coverage        ?   36.19%           
=======================================
  Files           ?        8           
  Lines           ?      442           
  Branches        ?        0           
=======================================
  Hits            ?      160           
  Misses          ?      282           
  Partials        ?        0           
Flag Coverage Δ
unittests 36.19% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TL231
Copy link
Contributor Author

TL231 commented Jul 5, 2023

Added some lines to prevent pytest and pylint from reporting the psi4 import errors, I don't think there's anymore changes to be made on my end if we're pushing off the psi4 dependencies resolution later. Also temporarily disabled sphinx doc checking in the workflow till Adrian finishes his docs, so I'll add that 1 line in when he's done.

One small note is that uploading coverage report sometimes fail due to API issues, their end not ours, so you can just retry the workflow till it works if anyone ever experiences that in the future.

@chmwzc chmwzc merged commit 559b4c4 into main Jul 6, 2023
@TL231 TL231 deleted the dev/poetry_workflow branch July 25, 2023 08:19
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.

4 participants