-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix(dependencies): move builds and dependencies to poetry #819
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
Conversation
|
Thank you for writing this! It is definitely a change that needed to be made at some point. We should keep requirements.txt imo. I tried getting it to autogenerate when I was making CI changes a little while ago, and it was messy, but if you have a better/clean way I am open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this get changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to change all of these notebooks to use the poetry, but this one still have some issues with the cache in the local setup which I am trying to solve it with @okhat , would be happy to chat if you can give me a hand on that.
|
@isaacbmiller thank you for the review and comments. I have added it to recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an easy way for people to verify that the notebooks run correctly? Seems like that might be hard to feasibly test. Can we add a CI test for it easily (out of scope for this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise just remove the line about notebooks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say we should make it somehow easy to test and run at least locally and right now I have an issue with running them locally with jupyter and the cache! did you set that up?
And I agree they should be moved to the CI, or I was thinking about move them as integration tests for the system in the pytest but still the same issue with setting up the tests to use the cache which I tried to fix here:
#606
|
Also, @okhat should be a blocking reviewer on this one, as he uploads the builds to pypi. I talked to him like a month ago about moving away from setup.py, and I thinking the testing wasn't good enough, but now it is significantly better |
|
I agree, but still the PR is a draft till I talk with @okhat |
ammirsm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say the only thing that make me stop is that why locally I can not setup running the notebooks, if you would give me the hand i think it should be gtg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say we should make it somehow easy to test and run at least locally and right now I have an issue with running them locally with jupyter and the cache! did you set that up?
And I agree they should be moved to the CI, or I was thinking about move them as integration tests for the system in the pytest but still the same issue with setting up the tests to use the cache which I tried to fix here:
#606
|
@isaacbmiller @okhat folks should I merge the master with this or we should wait on this one for now? |
isaacbmiller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall lgtm but I still think that @okhat should be a blocking reviewer on this
| @@ -1,1960 +1,1026 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this file get changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import should change and we make sure it is working with the poetry import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah forgot I asked above lol. lgtm
📝 Changes Description
This PR contains the following changes:
🛠 Type of Change
🔍 How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
How to test?
the way that you can test it is to setup an environment and follow the steps for setting up your poetry and use that.
we should run notebooks and make sure they works.
another part that should be tested is the build of the package and push it to pypi, which should be done with poetry build
To publish a Python package to PyPI using Poetry, you need to follow these steps:
Checklist
pyproject.tomlfilepoetry.lockfilerequirements.txtfile (till 2024-06-01)ruff check . --fix-onlyto appease the lint gods