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

Pip install resolution #20

Merged
merged 56 commits into from
Oct 23, 2023
Merged

Pip install resolution #20

merged 56 commits into from
Oct 23, 2023

Conversation

rajgane07
Copy link
Contributor

Qokit code uses few Json files for calculations, these have been added back

@rajgane07 rajgane07 temporarily deployed to pypi October 18, 2023 17:56 — with GitHub Actions Inactive
@rajgane07
Copy link
Contributor Author

This PR is to resolve bug, where some part of code will not work after pip install the repository either using pip install git+.. or pip install from pypi. The reason being during package time it configured to remove all the files under asset folder. The codes referencing some of the files will break.
Fix is move all the unwanted filed to archive folder, respective owners can check them and remove it. In the .toml only exclude the archived files.

Test this by either doing pip install git+ https://github.com/jpmorganchase/QOKit.git@test_coverage_circuit_optimization and/or pip install from pypi test site.
run following, it should not throw error.
from qokit.parameter_utils import get_fixed_gamma_beta
get_fixed_gamma_beta(3, 1)

@rsln-s
Copy link
Contributor

rsln-s commented Oct 18, 2023

Thank you, @rajgane07! @HaoTy, can you confirm that this resolves #19 on your end?

@rsln-s rsln-s linked an issue Oct 18, 2023 that may be closed by this pull request
Copy link
Collaborator

@HaoTy HaoTy left a comment

Choose a reason for hiding this comment

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

Yeah, this fixes #19 on my end. Thanks!

@rajgane07
Copy link
Contributor Author

Can you wait before merge? I want to check something

Copy link
Contributor

@rsln-s rsln-s left a comment

Choose a reason for hiding this comment

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

Install works on my machine! @rajgane07, could we make sure (using e.g. grep) that none of the file names under archives/ are mentioned anywhere in the code, and then delete the archives/ folder?

Looks great otherwise!

@rajgane07 rajgane07 temporarily deployed to pypi October 19, 2023 15:58 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 19, 2023 17:28 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 19, 2023 18:08 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 19, 2023 18:37 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 19, 2023 18:58 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 20, 2023 17:20 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 20, 2023 18:23 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 20, 2023 19:21 — with GitHub Actions Inactive
@rajgane07
Copy link
Contributor Author

During pip packaging, json files/.npy files were getting excluded. This change fixes that issue. @rsln-s I created archives file after verifying there are no depenency in the code. I have removed archives folder too in this change.

All can you please verify and review one more time?

@rajgane07 rajgane07 temporarily deployed to pypi October 20, 2023 20:02 — with GitHub Actions Inactive
@rajgane07 rajgane07 temporarily deployed to pypi October 20, 2023 20:51 — with GitHub Actions Inactive
@rsln-s rsln-s temporarily deployed to pypi October 23, 2023 13:24 — with GitHub Actions Inactive
Copy link
Contributor

@rsln-s rsln-s left a comment

Choose a reason for hiding this comment

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

LGTM! I confirmed that the install works locally. @rajgane07, feel free to merge

@rajgane07 rajgane07 merged commit ccd40b5 into main Oct 23, 2023
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.

pip install git+ causes error in asset-dependent parameter utils
4 participants