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.

Sign up for GitHub

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

Jump to bottom

CHIA-597: Use poetry for package management #11057

Merged
merged 166 commits into from
Jul 10, 2024
Merged

CHIA-597: Use poetry for package management #11057

merged 166 commits into from
Jul 10, 2024

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Apr 5, 2022 •
edited
Loading

Devs should continue to use the install.sh / Install.ps1 scripts for convenience. These scripts do the following:

Download and install poetry into .penv
Setup a chia-blockchain specific .venv
Create a symlink between venv and .venv

You can then use activate in the same way as before

This approach keeps the poetry deployment in .penv separate from the chia-blockchain install in .venv. If you want to run the poetry command you will need to run it from the .penv directory.

You can install poetry globally if you want, but then you may need to pay attention to the versioning in poetry specifically (running a different version from the one installed in install.sh may have unexpected results). We are still exploring options on better ways to handle the poetry install.

Draft for:

@altendky
preliminary setup with poetry
f6e6384
@altendky
Merge branch 'main' into poetry
41b1047
@altendky
don't worry about locking the poetry env for the exploration right now
e1baf5c
@altendky
recover black config in pyproject.toml
0e3ca4c
@altendky
install.sh for linting workflow
8eb66ca
@altendky
pull chiavdf version from poetry in install-timelord.sh
1d4c1d1
@altendky
activate etc in upload and lint workflow
fb57f56
@altendky
debug poetry show output on macos
c17f767
@altendky
update to clvm-tools 0.4.4
e862f9b
@altendky
Merge branch 'main' into poetry
050cc24
@altendky
Merge branch 'main' into poetry
4591ff5
@altendky
Merge branch 'main' into poetry
7e9c103
@altendky
add back setproctitle
77585c5
@altendky
Merge branch 'main' into poetry
4e59137
@altendky
catchup deps
88b9aaa
@altendky
lock
a8b7c03
@altendky
Merge branch 'main' into poetry
701a834
@altendky
allow 3.10
92c8204
@altendky
debug
98fdc43
@altendky
make Install.ps1 use poetry as well
70dd4c9
@altendky
-ErrorAction Ignore
9c96bfe
@altendky
more todo about python version selection
3adf176
@altendky
make install-timelord.sh modifications more posix compliant
a02af91
@altendky
gitignore .penv* and .venv* as well
90faa5b
@altendky
remove requirements.penv.txt
608eace
@altendky
get artifact check requirements from poetry
6273eb5
@altendky
fix .gitignore for venv symlink
27fcd23
@altendky
separate out poetry setup
398821e
@altendky
:
021b9d0
@altendky
comment out code to be worked on
e6f1f6c
@github-actions GitHub Actions
Copy link
Contributor

File Coverage Missing Lines
chia/_tests/core/data_layer/util.py 62.5% lines 143, 160-161
Total Missing Coverage
9 lines 3 lines 66%

@emlowe emlowe marked this pull request as ready for review July 8, 2024 17:15
@emlowe emlowe requested a review from a team as a code owner July 8, 2024 17:15
@emlowe
Copy link
Contributor

emlowe commented Jul 8, 2024

@SocketSecurity ignore pypi/[email protected]
@SocketSecurity ignore pypi/[email protected]

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 8, 2024

File Coverage Missing Lines
chia/_tests/core/data_layer/util.py 62.5% lines 143, 160-161
Total Missing Coverage
9 lines 3 lines 66%

@arvidn
Copy link
Contributor

arvidn commented Jul 8, 2024

when testing this on Mac (M1) I get this warning:

Warning: The file chosen for install of twine 5.1.0 (twine-5.1.0-py3-none-any.whl) is yanked. Reason for being yanked: https://github.com/pypa/twine/issues/1125

I suppose this is related to #18253

arvidn
arvidn reviewed Jul 8, 2024
View reviewed changes
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks good. It would be nice to have some documented guidance on how treat the lockfile, and PRs making changes to it. I assume it would be accepted with caution, as we want to lock down our supply-chain risk.

arvidn
arvidn previously approved these changes Jul 8, 2024
View reviewed changes
@Starttoaster
Copy link
Contributor

Waiting on coverage-diff override

@emlowe emlowe closed this Jul 8, 2024
@emlowe emlowe reopened this Jul 8, 2024
@emlowe
Copy link
Contributor

emlowe commented Jul 8, 2024

Sorry Clicked the wrong thing and ended up closing and reopening the PR

@emlowe
Copy link
Contributor

emlowe commented Jul 8, 2024

looks good. It would be nice to have some documented guidance on how treat the lockfile, and PRs making changes to it. I assume it would be accepted with caution, as we want to lock down our supply-chain risk.

Yes, generally you will want to use poetry to make any changes, eg:

.penv/bin/poetry update urllib3

However, I'm also hopeful that now all the dependencies are listed in the lock file, we can relay on dependabot creating PRs to update anything

@emlowe
Copy link
Contributor

emlowe commented Jul 8, 2024

Well, dependabot seems to have a flow in that it updates the lock files but not the toml (dependabot/dependabot-core#8603)

So that might take some manual effort per the reporter until that gets resolved....

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 8, 2024

File Coverage Missing Lines
chia/_tests/core/data_layer/util.py 62.5% lines 143, 160-161
Total Missing Coverage
9 lines 3 lines 66%

@coveralls-official Coveralls Official
Copy link

coveralls-official bot commented Jul 8, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9860288518

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 6 of 9 (66.67%) changed or added relevant lines in 2 files are covered.
  • 39 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.03%) to 90.955%
Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/_tests/core/data_layer/util.py 5 8 62.5%
Files with Coverage Reduction New Missed Lines %
chia/wallet/wallet_node.py 1 88.72%
chia/_tests/util/test_build_job_matrix.py 1 96.77%
chia/daemon/client.py 1 73.94%
chia/introducer/introducer.py 1 78.26%
chia/data_layer/data_layer_wallet.py 2 91.93%
chia/server/node_discovery.py 4 79.08%
chia/server/address_manager.py 7 90.48%
chia/_tests/core/util/test_lockfile.py 22 77.73%
Totals Coverage Status
Change from base Build 9846325110: -0.03%
Covered Lines: 101594
Relevant Lines: 111650
💛 - Coveralls

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 9, 2024

File Coverage Missing Lines
chia/_tests/core/data_layer/util.py 62.5% lines 143, 160-161
Total Missing Coverage
9 lines 3 lines 66%

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 9, 2024

File Coverage Missing Lines
chia/_tests/core/data_layer/util.py 62.5% lines 143, 160-161
Total Missing Coverage
9 lines 3 lines 66%

@Starttoaster Starttoaster merged commit 6d9219b into main Jul 10, 2024
369 of 370 checks passed
@Starttoaster Starttoaster deleted the poetry branch July 10, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arvidn arvidn arvidn approved these changes

Assignees

@altendky altendky

Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@altendky @emlowe @arvidn @Starttoaster