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

Replacing requirement files and setup.py with pyproject.toml #70

Merged
merged 22 commits into from
Jun 29, 2023

Conversation

robotAstray
Copy link
Contributor

@robotAstray robotAstray commented Jun 4, 2023

Summary

This pull request aims to resolve issue 61. The requirement files and setup.py have been replaced with pyproject.toml.

Details and comments

The pyproject.toml in this repo is a template and contains the required fields. In addition, optional fields can be added according to the documentation found here.
To install pyproject.toml use the command python -m pip install path/to/pyproject.toml

@CLAassistant
Copy link

CLAassistant commented Jun 4, 2023

CLA assistant check
All committers have signed the CLA.

@robotAstray
Copy link
Contributor Author

robotAstray commented Jun 5, 2023

Hello @caleb-johnson, would you kindly have a look at my solution?
If there are any problems I will fix them.
Thank you

Copy link
Collaborator

@caleb-johnson caleb-johnson left a comment

Choose a reason for hiding this comment

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

Thanks for this submission! Just a small comment.

pyproject.toml Outdated Show resolved Hide resolved
@garrison
Copy link
Member

garrison commented Jun 5, 2023

I'm going to approve the workflow, but my guess is that the "minimum version tests" and "development version tests" will fail for the meantime. Right now, an improved, pyproject.toml-compatible version of these tests is at https://github.com/Qiskit-Extensions/circuit-knitting-toolbox/blob/main/tools/extremal-python-dependencies.py. I'd really like to spin that off into its own repository so it can be used in other projects without having code duplicated in all of them.

Co-authored-by: Jim Garrison <jim@garrison.cc>
@robotAstray
Copy link
Contributor Author

I'm going to approve the workflow, but my guess is that the "minimum version tests" and "development version tests" will fail for the meantime. Right now, an improved, pyproject.toml-compatible version of these tests is at https://github.com/Qiskit-Extensions/circuit-knitting-toolbox/blob/main/tools/extremal-python-dependencies.py. I'd really like to spin that off into its own repository so it can be used in other projects without having code duplicated in all of them.

@garrison, yes, I agree. The checks will not pass because requirements.txt and setup.py have been replaced by pyproject.toml. Some of the checks are looking for the paths of the files I removed (i.e. requirments.txt and setup.py).

caleb-johnson
caleb-johnson previously approved these changes Jun 5, 2023
Copy link
Collaborator

@caleb-johnson caleb-johnson left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@caleb-johnson
Copy link
Collaborator

Ahh the CI was passing earlier, sorry I'm getting caught up now :)

@caleb-johnson caleb-johnson dismissed their stale review June 5, 2023 22:52

Broken CI

@robotAstray
Copy link
Contributor Author

@garrison should I re-add requirments.txt and setup.py to the repo just so the checks pass? The idea was to replace them so I removed them, but I can see without them the checks will not be successful.

@garrison
Copy link
Member

garrison commented Jun 6, 2023

I think it makes more sense just to remove the dev and min version test workflows for now.

With regards to setup.py, I think it makes sense to have a minimal one for other reasons. Something identical to https://github.com/Qiskit-Extensions/circuit-knitting-toolbox/blob/contributing.md/setup.py would suffice.

@robotAstray
Copy link
Contributor Author

@garrison ok, that is done for you. Thank you.

setup.py Outdated Show resolved Hide resolved
@garrison
Copy link
Member

garrison commented Jun 6, 2023

tox.ini is going to need some changes, too. Where you see

deps = -rrequirements.txt
-rrequirements-dev.txt

replace with extras = dev

Might also be worth grepping the repository (e.g., git grep) to see if the requirements files are mentioned in any other places.

@robotAstray
Copy link
Contributor Author

Aside for the dev and min version test workflows which I removed, the files containing requirement files are:
ecosystem.json with :

        "dependencies_files": [
           "requirements.txt",
           "requirements-dev.txt"
       ],
  • README.md (I commented out the two paragraphs about the dev and min version test workflows since the files have been removed).
  • file-map-and-description.md (I can fix this by adding a short description of pyproject.toml)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5192209113

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 4898908272: 0.0%
Covered Lines: 14
Relevant Lines: 14

💛 - Coveralls

@robotAstray
Copy link
Contributor Author

robotAstray commented Jun 7, 2023

@garrison should I modify theecosystem.json dependencies_files entry? If so what should I replaced that part with?
https://github.com/robotAstray/quantum-prototype-template/blob/be40dfe47e8cf5eafc43e3b553feb1ffd617c20c/ecosystem.json

@robotAstray
Copy link
Contributor Author

robotAstray commented Jun 14, 2023

I just ran tox -elint from the root folder and I have this

$ tox -elint
lint: commands[0]> black --check .
lint: failed with black is not allowed, use allowlist_externals to allow it
  lint: FAIL code 1 (0.28 seconds)
  evaluation failed :( (1.58 seconds)

@garrison
Copy link
Member

You will need extras = dev in the lint and black environments, too.

@robotAstray
Copy link
Contributor Author

Hello @garrison we already have extrs = dev on line 15 in tox.ini . I guess when you refer to lint and black environments you are referring to the block [testenv:black] and [testenv:lint]? I have tried to add extras = dev to these blocks and I still get the same error.

@garrison
Copy link
Member

What version of tox are you using? If below version 4, you might have to pass the -r flag to tox for it to re-create the environment.

@robotAstray
Copy link
Contributor Author

@garrison, I am currently on version 4.6.0.

@robotAstray
Copy link
Contributor Author

robotAstray commented Jun 16, 2023

perhaps there is a problem with my system. I ran all the commands manually and it seems to me the checks all passed
Update: 16th June 02:39 (CEST): I have created a virtual environment and re-installed pyproject.toml. That resolved some of the issues.
I have added allowlist_externals = pylint, nbqa under [testenv:lint] to allow tox -elint to run correctly.
Update: 16th June 03:17 (CEST): I think I have resolved the problems related to this issue. Let me know if there is anything else to fix (or change). Thank you.

tox.ini Outdated Show resolved Hide resolved
thank you! I understand.

Co-authored-by: Jim Garrison <jim@garrison.cc>
Comment on lines 27 to 30
<!-- - [requirements.txt](../requirements.txt) - list of required 3rd party packages to run your project.
- [requirements-dev.txt](../requirements-dev.txt) - list of required 3rd party packages that are
NOT required to run your project, but which might benefit developers. It can include specific test
libraries, style checks packages etc.
libraries, style checks packages etc. -->
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove these lines containing requirements.txt and requirements-dev.txt? (since there is no chance in them coming back)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That has been resolved.

Copy link
Member

@garrison garrison Jun 29, 2023

Choose a reason for hiding this comment

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

Can you double check? I think you removed a different commented region, actually, than this one in c3a3519. I was thinking it makes sense to keep the explanations in README.md commented out since (we hope) the development and minimum version tests will re-appear in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @garrison you can check my last push a4a72f7 it contains the changes you requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also re-added the comment section I wrongly removed previously. Sorry about that.

robotAstray and others added 3 commits June 29, 2023 14:00
Co-authored-by: Jim Garrison <jim@garrison.cc>
* Update README.md

adding comment section that was removed by mistake

* Update README.md

re-adding comment section

* Update file-map-and-description.md

removing comments referring to `requirments.txt`
@garrison
Copy link
Member

Just one note for the future (don't worry about this here): typically it is customary to make pull requests from a new branch in your fork, rather than using main in your fork. Using main can sometimes make things confusing, particularly if you open up multiple PRs to the same repository.

Copy link
Member

@garrison garrison left a comment

Choose a reason for hiding this comment

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

Thanks, this is looking very good to me. I appreciate the contribution!

Co-authored-by: Jim Garrison <jim@garrison.cc>
@robotAstray
Copy link
Contributor Author

Just one note for the future (don't worry about this here): typically it is customary to make pull requests from a new branch in your fork, rather than using main in your fork. Using main can sometimes make things confusing, particularly if you open up multiple PRs to the same repository.

noted!

@garrison garrison merged commit 48015c1 into qiskit-community:main Jun 29, 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.

Replace requirements txt files and setup.py with pyproject.toml
5 participants