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

remove fastapi dep to solve #47 #54

Merged
merged 5 commits into from
May 3, 2024
Merged

remove fastapi dep to solve #47 #54

merged 5 commits into from
May 3, 2024

Conversation

xiangchenjhu
Copy link
Collaborator

@xiangchenjhu xiangchenjhu commented May 1, 2024

remove unnecessary dependent
resolves #47

@xiangchenjhu xiangchenjhu requested a review from amitschang May 1, 2024 16:34
@xiangchenjhu xiangchenjhu requested a review from a team as a code owner May 1, 2024 16:34
@@ -5,5 +5,4 @@ ray[default]==2.9.3
rdkit_pypi==2022.9.5
torch==2.2.0
torch_geometric==2.5.2
fastapi==0.110.0
uvicorn[standard]==0.29.0
Copy link
Member

Choose a reason for hiding this comment

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

Do you still need this or can it get nuked also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FastAPI can be removed as it is not in use in Bluephos project.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, the above inline comment was on line 8 in reference to uvicorn, which is probably only needed for fastapi. If this is true, it should get removed in this PR also, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I've also removed Unicorn after checking, and I've updated the PR accordingly.
Thanks!

@xiangchenjhu xiangchenjhu requested a review from jamienoss May 1, 2024 17:55
@jamienoss
Copy link
Member

@xiangchenjhu I see that there's an extra three commits to update with main and resolve conflicts, FYI, these shouldn't have been necessary. A good practice for updating your branch with main is to do the following (with conflicts in requirements/prd.txt):

git fetch origin
git rebase origin/main
# resolves conflicts in requirements/prd.txt
git add requirements/prd.txt
git rebase --continue
git push origin del-fastapi-dep
# above should fail
git push origin del-fastapi-dep -f

If not familiar with rebasing, I always advise creating a backup of your branch first using (assuming already on branch)

git branch del-fastapi-dep.bkup

@xiangchenjhu
Copy link
Collaborator Author

xiangchenjhu commented May 1, 2024

Thank you @jamienoss . I just noticed this helpful suggestion. It should be handy for future use.

@xiangchenjhu xiangchenjhu merged commit 55d3bf7 into main May 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants