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

poetry dependency management #150

Merged
merged 23 commits into from Mar 22, 2021
Merged

poetry dependency management #150

merged 23 commits into from Mar 22, 2021

Conversation

v-goncharenko
Copy link
Collaborator

@v-goncharenko v-goncharenko commented Mar 11, 2021

I moved package management to poetry as I suggested in #146

Separated development dependencies from direct ones. Run tests, evaluated run.py and built docs (here got some errors not related to dependencies).

I've investigated requirements.txt and found funny things:

  • pillow and patool are not used anywhere, so I skipped adding them to poetry setup
  • pyunpack used only once and could be easily be replaced with standard library call (I'll open an issue later, or fix it directly with PR)
  • m2r is no longer supported, so I changed it to m2r2 which is a supported fork
  • numpy, scipy and Sphinx are explicitly used but was not placed to explicit dependencies, fixed it

Last but not least, I'll describe poetry (as well as development env) setup procedure in CONTRIBUTING file in another PR. (this requires #151 to be merged to avoid conflicts)

@v-goncharenko
Copy link
Collaborator Author

v-goncharenko commented Mar 11, 2021

Another thing I've noticed - need to update Actions pipelines (switch them to poetry too). I'll do that a bit later

@v-goncharenko v-goncharenko linked an issue Mar 12, 2021 that may be closed by this pull request
@v-goncharenko v-goncharenko marked this pull request as ready for review March 12, 2021 08:57
@v-goncharenko
Copy link
Collaborator Author

Finished!

We are completely ready to move to stable explicit crossplatform dependencies =)

@sylvchev
Copy link
Member

Thank you ! This PR is really nice.

Regarding some specific dependencies :

I've investigated requirements.txt and found funny things:

* `pillow` and `patool` are not used anywhere, so I skipped adding them to poetry setup

* `pyunpack` used only once and could be easily be replaced with standard library call (I'll open an issue later, or fix it directly with PR)

pyunpack is used to unpack archive files for Wang2016 dataset. It is a frontend that need some backend packages to work correctly, and patool is the "backend" package used to unpack the required archive. If you could replace it with a standard library call, this could help to reduce the dependency to other package. For now, I think you need reintegrate patool in the requirements.

Last but not least, I'll describe poetry (as well as development env) setup procedure in CONTRIBUTING file in another PR. (this requires #151 to be merged to avoid conflicts)

This is a good idea to update the contributing page.

@v-goncharenko
Copy link
Collaborator Author

Done

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@v-goncharenko
Copy link
Collaborator Author

v-goncharenko commented Mar 21, 2021

@sylvchev I've added setup instructions to contribution file. (and also fixed the issues you pointed in change request)

I guess this makes this PR mergeable.

Copy link
Member

@sylvchev sylvchev left a comment

Choose a reason for hiding this comment

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

Just two typos to fix and we could merge!

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@sylvchev sylvchev merged commit 2f3dee6 into master Mar 22, 2021
@v-goncharenko
Copy link
Collaborator Author

Great!

Then navigate straight to PyPI!
I'll checkout what is needed for that and let's discuss it during office hours.

@v-goncharenko v-goncharenko deleted the poetry branch March 23, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency management
2 participants