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

requests not included in setup.py #1122

Closed
rob-metalinkage opened this issue Jun 23, 2020 · 7 comments · Fixed by #1175
Closed

requests not included in setup.py #1122

rob-metalinkage opened this issue Jun 23, 2020 · 7 comments · Fixed by #1175

Comments

@rob-metalinkage
Copy link

although requests is defined in requirements.txt it is not in setup.py and hence not picked up by pip install

@tgbugs
Copy link
Contributor

tgbugs commented Jul 20, 2020

It should not be. See #969 and #1035. If you need requests install via pip install rdflib[sparql].

@rob-metalinkage
Copy link
Author

See the direction, fair enough...

but a few things seem off -

  1. there seems to be inconsistency between requirements.txt and setup.py (and not sure if this is a good thing to have similar info in both places anyway?)
  2. Parsing a graph from a HTTP request has nothing to do with SPARQL - so this isnt very intuitive a solution
  3. probably the parse module should report a sensible error with the correct import suggestion if you ask it for a HTTP source and it doesnt have the install available
  4. the documentation for parse is quite explicit HTTP is supported without any mention of the requirement.

I think all these things need to be addressed for the proposed solution to be acceptible.

@tgbugs
Copy link
Contributor

tgbugs commented Jul 21, 2020

As I mentioned on the pr, rdflib doesn't use requests for fetching http resources (unless I missed something). If I indeed did not miss something, then we should probably remove requests from requirements.txt since I suspect that @gromgull originally included it for testing purposes, and that is now covered by tests_require.

@white-gecko
Copy link
Member

white-gecko commented Aug 14, 2020

Isn't the requirements.txt just for development purposes? So in my eyes it is ok that setup.py and requirements.txt are not the same.

@FlorianLudwig
Copy link
Contributor

@rob-metalinkage

Parsing a graph from a HTTP request has nothing to do with SPARQL - so this isnt very intuitive a solution

That is not what requests is used for in rdflib. It is needed to support service queries, see: Sparql 1.1 Federated Query Extension

@white-gecko Usually the requiremens.txt is used for version pinning. Which is important to move towards reproducibility. This means it would:

  1. contain exact versions for every package
  2. contain all dependencies - including dependencies of dependencies.

Neither is the case with the current files.

From my experience managing those is somewhat of a PITA. So personally I moved to pipenv or poetry to manage dependencies.

@white-gecko
Copy link
Member

Ok I did not know that. On the page for pipenv it says:

Managing a requirements.txt file can be problematic, so Pipenv uses the upcoming Pipfile and Pipfile.lock instead, which is superior for basic use cases.

So this might be the future. I assume poetry has similar features.

This might be a bigger discussion with @ashleysommer and @nicholascar involved if and how we would like to change the dependency management.

@nicholascar
Copy link
Member

Hi all, I have a PR which I'll put in early next week that removes requests from RDFlib. This is possible for in the two places that it's used, its functionality can be replaced with urllib.

So this will deal with the immediate issue of having it listed in setup.py/requirements.txt and is good practice anyway and that PR will close this issue. Whether we move to poetry etc is a separate concern (@ashleysommer and I have briefly talked about it but haven't really done anything there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants