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

infra: move requirements to a pyproject file and set up hatch env #440

Merged
merged 15 commits into from
Dec 5, 2023

Conversation

math411
Copy link
Contributor

@math411 math411 commented Oct 31, 2023

Issue #, if available:

Description of changes:

This pyproject file uses hatch for management. With this, we can define testing environments including the default one which is being used here. Currently, this is a sloppy implementation and generalizes for our use case. There are a few improvements to be done based on this:

  1. style checks and linting
    1.1 We can use ipynb specific linters. Still researching this to properly propose something.
  2. Add a test env for using the requirements file. While we have great confidence around the dependencies working based around testing from the constrained dependencies defined in this file, it would provide better confidence if we also tested with those dependencies. This would be a script included and whether it is to be included in the test executions is a later question.
  3. Updating the README for the new testing plan. Commands are slightly changed. For instance, hatch run test takes case of the dependency setup and utilizes the constraints in the pyproject file. A user can still run pip install -e . to install these dependencies if they wish to use other tools besides hatch.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@math411 math411 added enhancement New feature or request dependencies Pull requests that update a dependency file labels Oct 31, 2023
@math411 math411 marked this pull request as ready for review November 10, 2023 18:22
@math411 math411 requested a review from a team as a code owner November 10, 2023 18:22
@math411 math411 requested a review from krneta November 10, 2023 18:22
@math411
Copy link
Contributor Author

math411 commented Nov 15, 2023

This will use hatch as a trial of the tooling for Braket. This is a 2 way door and a follow up PR can be released to remove this should the case arise.

@@ -0,0 +1,134 @@
[build-system]
requires = ["hatchling", "hatch-requirements-txt"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these files? Will they be included in this 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.

pyproject.toml Outdated
"typing",
]

[tool.black]
Copy link
Contributor

Choose a reason for hiding this comment

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

Where will this actually be applied? In the notebooks or in the .py files that notebooks use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not applied currently. I can remove it to avoid confusion but I figured it would be alright to leave in for people to use and build off of.

pyproject.toml Outdated
[tool.ruff]
target-version = "py39"
line-length = 120
select = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you get this list from? Again, not sure where/how this applies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was from hatch --init. I have left this as a linting starting point. I can remove to avoid confusion.

@@ -1,19 +0,0 @@
amazon-braket-sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we no longer need this file? Are we just merging this with the requirements file for NBIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was to address the discussed proposal around not installing unpinned dependencies and instead using just the requirements dependencies.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@math411 math411 requested a review from krneta November 18, 2023 01:16
@math411 math411 merged commit 2ed78de into main Dec 5, 2023
8 checks passed
@math411 math411 deleted the pyproject branch December 5, 2023 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants