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

Unnecessary dependency on flask #47

Closed
kshitij12345 opened this issue Jan 9, 2021 · 2 comments · Fixed by #65
Closed

Unnecessary dependency on flask #47

kshitij12345 opened this issue Jan 9, 2021 · 2 comments · Fixed by #65
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@kshitij12345
Copy link

From what I see, flask is only used in the example file modelAPI.py in RESTAPI.

The core code is not at all dependent on flask, however it is a required dependency

I think it would be better to just add note in modelAPI.py to install flask and remove it from requirements.txt.

Thanks for the interesting project.

@kshitij12345 kshitij12345 added the bug Something isn't working label Jan 9, 2021
@R1j1t
Copy link
Owner

R1j1t commented Jan 9, 2021

That is a valid point. My reasoning for not removing Flask was, this repository has a dependency on it. Core functionality dependency is better captured in setup.py below:

install_requires=["torch", "editdistance", "transformers", "spacy"],

I would not mind having a requirement.txt in RESTAPI folder. But I was not sure so I just left it. I will check on the best practice and then maybe change it.

And, I am happy you liked the project! Please feel free to contribute!

@R1j1t R1j1t added the documentation Improvements or additions to documentation label Jan 9, 2021
@R1j1t
Copy link
Owner

R1j1t commented Feb 17, 2021

I tried to find any relevant PEP to identify what is the recommended way of using requirement.txt but was not able to find anything specific.

PEP 518 talks about minimum build requirement in setup.py (which this follows). I think the discussion is valid and I will be happy to create requirement.txt in RESTAPI/examples. My decision is biased based on a similar experience I had when one of the optional dependencies choked the CI and I had to comment that package. @kshitij12345 do you want to pickup this change?

Other similar PEP but not much relevant:
PEP 508
PEP 650

R1j1t added a commit that referenced this issue Aug 22, 2021
@R1j1t R1j1t closed this as completed in #65 Aug 22, 2021
R1j1t added a commit that referenced this issue Aug 22, 2021
* fixes #47

* update doc task list, completed by #52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants