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

Simplify toml file #456

Open
purva-thakre opened this issue Feb 1, 2024 · 12 comments
Open

Simplify toml file #456

purva-thakre opened this issue Feb 1, 2024 · 12 comments
Labels
devops enhancement New feature or request github_actions Pull requests that update GitHub Actions code
Milestone

Comments

@purva-thakre
Copy link
Collaborator

#455 shows the installation needs for different workflows/jobs are becoming bloated.

It might be better to use something like dependency groups to simplify what installation is needed for which job.
https://python-poetry.org/docs/pyproject/#dependencies-and-dependency-groups

We also have a separate requirements file for docs. It might be better to figure out a way to condense these into the toml file.

If the above is possible then we will also need to simplify the workflows for Github Actions.

@purva-thakre purva-thakre added enhancement New feature or request devops github_actions Pull requests that update GitHub Actions code labels Feb 1, 2024
@purva-thakre purva-thakre self-assigned this Feb 1, 2024
@vprusso
Copy link
Owner

vprusso commented Apr 20, 2024

On this note, do we want to specify the requirements in requirements.txt files and use the .toml file for other things (for instance, I like how this is done on mitiq):
https://github.com/unitaryfund/mitiq/blob/main/pyproject.toml

I also don't like how we are pinning to much older versions of thing (like numpy as we should be updating to use the most up-to-date versions of these libraries to take advantage of their optimizations, etc.)

Any thoughts, @purva-thakre ?

@purva-thakre
Copy link
Collaborator Author

I also don't like how we are pinning to much older versions of thing (like numpy as we should be updating to use the most up-to-date versions of these libraries

Even if we pin these, dependabot does create a PR to the update to the lastest version. I double-checked, we are using the latest version of numpy.

@vprusso
Copy link
Owner

vprusso commented Apr 21, 2024

Ah okay, that's good. I wonder then if it makes more sense to use the >= pattern to be more explicit about the fact that it is indeed using the more recent version. It seems a bit confusing that it's pinned to an older version but uses a newer one. Although, perhaps this is standard? (i.e. I think mitiq does this as well).

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Apr 22, 2024

I wonder then if it makes more sense to use the >= pattern

We used to use a version of this. I changed it in #204 or #218 because I noticed some local failures when the latest installed dependency versions were not compatible with toqito. But these changes were before we started using dependabot and a better CI workflow.

I could give the pattern a try again since we can also catch failures like these through PRs.

@vprusso
Copy link
Owner

vprusso commented Apr 22, 2024

We could put that as a "thing to investigate" for this issue. I honestly don't know if doing it how we are is standard or if it's misleading to pin to a lower version if a higher one is being used. My thoughts would be that this would be misleading, but we can use this issue to look into that as well.

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Apr 22, 2024

I think tilde pattern could work well for showing that we use the recent version of that dependency.

https://stackoverflow.com/questions/54720072/dependency-version-syntax-for-python-poetry/54720073#54720073

The issue with >= is we have to specify the higher bound to make sure poetry does not install the latest version of a dependency that's not compatible with our package.

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented May 1, 2024

On this note, do we want to specify the requirements in requirements.txt files and use the .toml file for other things

The problem with this is pip does not do a good job of dependency resolution. Last year, pip was installing versions of dependencies that were not compatible with toqito, then the tests would fail locally etc. When I switched to using poetry, it resolved to a better version number.

My preference would be to keep the toml file for dev dependencies at least. We could use requirements.txt for people who don't need to install the editable versions.

@purva-thakre purva-thakre added this to the v1.0.8 milestone May 1, 2024
@purva-thakre
Copy link
Collaborator Author

purva-thakre commented May 2, 2024

Discussed with Vincent to keep the toml file for dependencies. The categories for the toml file could be- dev, lint, tests, and docs.

Skip the requirements.txt file for now. Figure out later what gets installed with the non-editable version.

@purva-thakre
Copy link
Collaborator Author

purva-thakre commented May 8, 2024

@vprusso Do we use the toml file to pack toqito into a sdist or wheel? I think the answer is no because it is currently used only for dependency management. I remember you had used the setup.py file for this.

I am thinking of adding package-mode = false in the toml file.
https://python-poetry.org/docs/basic-usage/#operating-modes

@vprusso
Copy link
Owner

vprusso commented May 11, 2024

Right now, I use setup.py to package things. However, maybe it would be better to eventually remove setup.py and just use the toml file to handle everything (including the sdist/wheel packaging).

@purva-thakre
Copy link
Collaborator Author

Ok. I'll use this issue to get rid of the setup.py file as well.

I vaguely remember the release process. So, I will send you a link to a Google doc over the next few days related to this process. My notes for the use of setup.py vs toml file before a release should be in it. Feel free to correct things.

@purva-thakre
Copy link
Collaborator Author

Dependency groups were added in #585

This issue is kept open to figure out a way to use the toml file in place of setup.py.

@purva-thakre purva-thakre modified the milestones: v1.0.8, v1.0.9 Jun 22, 2024
@purva-thakre purva-thakre removed their assignment Oct 8, 2024
@purva-thakre purva-thakre modified the milestones: v1.1.0, > v1.1.0 Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops enhancement New feature or request github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

No branches or pull requests

2 participants