adafruit / Adafruit_CircuitPython_Requests

Requests-like interface for web interfacing
MIT License
51 stars 37 forks source link

Simple async http requests #118

Open alexmherrmann opened 1 year ago

alexmherrmann commented 1 year ago

This will never go anywhere unless there's a pull request but I know this one isn't great yet, happy to work on it when possible as I think it's useful.

I took some of @dhalbert 's code that made async requests a thing and tried to write a small test and pull out some other experimental changes made. It won't do much until the socket read is also async but that is beyond my pay grade at the moment.

alexmherrmann commented 1 year ago

I will merge in the original so hopefully the merge conflict goes away.

FoamyGuy commented 1 year ago

In order for this to live in this repo it would need to be converted to a directory instead of single .py files. Some of the tooling that is used to generate library bundles and other things assumes that there will be only a single .py file, or a single directory containing the library .py files in the root of the repo.

Another option could be to create a seperate repo for the async version, then it could live in it's own repo as a single file library.

alexmherrmann commented 1 year ago

Thank you @FoamyGuy, I am very new to circuitpython obviously, would you recommend one way over the other? Perhaps @dhalbert has an opinion as it was originally his code.

Is there an example library with multiple .py files so I can get it right if it's the first way you mentioned?

FoamyGuy commented 1 year ago

I'm not certain which direction would be best. But I've added a note about it to discuss during our weekly meeting which takes place on discord this afternoon. It's open to the public if you're interested in joining or listening in. I'll get feedback from the team and report back here.

There are examples of the directory repos, if that ends up being the case I will link you one that can be used as reference.

tekktrik commented 1 year ago

@FoamyGuy correct me if I'm wrong, but the decision was to have this been it's own library. Not sure how we should start this process then with regards to this pull request

FoamyGuy commented 1 year ago

That is correct. Sorry I meant to comment back here after it was discussed but didn't do it.

The discussion is here: https://youtu.be/n8Y6tT6nSig?t=3032 and yep conses was ultimately to make a new library but be clear that it should keep the same functionality as this one, i.e. new things should get added to both instead of just one.

@alexmherrmann If you want to take what you have and put it into a new repo to get us kicked off that would be awesome. If your up for it there is a tool called cookie-cutter that we use to create the new libraries, it and how to use it are documented here: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/creating-a-library. If you'd rather not go through that process, that is okay as well. Make a new repo with the async code and ideally at least one example in it and link that here. We can run the cookie cutter and move your code into the generated library files afterward.

alexmherrmann commented 1 year ago

Thank you all, I should have some time this upcoming week. I haven't quite got the best way to write tests yet. Between the mocks and what I've seen in other repository it is still a little foreign to me but I'll try my best.

There were a few places it could block in the code too that would be useful to note. The socket read was the big one but I think @dhalbert had a way around that.

Are there requirements around documentation? Or is that all in the cookie cutter library too?

On Sat, Nov 12, 2022, 8:08 AM foamyguy @.***> wrote:

That is correct. Sorry I meant to comment back here after it was discussed but didn't do it.

The discussion is here: https://youtu.be/n8Y6tT6nSig?t=3032 and yep conses was ultimately to make a new library but be clear that it should keep the same functionality as this one, i.e. new things should get added to both instead of just one.

@alexmherrmann https://github.com/alexmherrmann If you want to take what you have and put it into a new repo to get us kicked off that would be awesome. If your up for it there is a tool called cookie-cutter that we use to create the new libraries, it and how to use it are documented here: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/creating-a-library. If you'd rather not go through that process, that is okay as well. Make a new repo with the async code and ideally at least one example in it and link that here. We can run the cookie cutter and move your code into the generated library files afterward.

— Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_CircuitPython_Requests/pull/118#issuecomment-1312502131, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIL3D5E2HCOJUQZMCPVLSTWH6XGXANCNFSM6AAAAAARQGL3KA . You are receiving this because you were mentioned.Message ID: @.***>

FoamyGuy commented 1 year ago

@alexmherrmann cookie-cutter will prompt you with yes/no whether you want it to generate the docs files. All official libraries choose yes for that option.

The content for the docs pages is taken from the docstrings in the code. We use a tool called sphinx to build it into html pages from those docstrings. This page talks about that process a bit: https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/sharing-our-docs-on-readthedocs and shows the command to run to make a build locally. Much of the page deals with setting up Readthe docs specifically which is something that an Adafruit team member would do once this repo is created, it requires permissions within a few different systems.

tekktrik commented 1 year ago

@alexmherrmann is this still something you're working on? I'm happy to run the cookiecutter if you'd like and get the library infrastructure set up :)

veloyage commented 10 months ago

Is this still being worked on, by anyone? Is there no interest any more, or did circumstances change? I would be quite interested in async requests, as currently they hang my UI. Only for a second or so, but still ugly. As wifi becomes a default peripheral, and internet connected projects the norm, I imagine this will become a common annoyance.

Wondering whether the library structure and cookiecutter stuff was the main issue, I tried to just run the existing code and ran into errors. Also, early in the thread @alexmherrmann said "It won't do much until the socket read is also async but that is beyond my pay grade at the moment." - that's not just above my paygrade, I am not even sure what that implies. Is significant heavy lifting required to make this useful?

Would be glad if someone with a better overview could enlighten me. I am happy to help, but clueless as to how.

dhalbert commented 10 months ago

"It won't do much until the socket read is also async but that is beyond my pay grade at the moment." - that's not just above my paygrade, I am not even sure what that implies. Is significant heavy lifting required to make this useful?

That is a significant addition - I think we need core CircuitPython work for that. A lot of the infrastructure is in place but it hasn't be done yet.

alexmherrmann commented 10 months ago

Hello all, my job has changed up quite a bit for the better and I'm back around and able to start helping. I was going to get main merged and updates in to the branch to make sure what was working before is still working now. We're moving into a house so things will be chaotic but the magtag is getting plugged back in as soon as we settle!

What @dhalbert is referring to, and can speak much more confidently about is that we can add as much async code in as we want at this level. However, (as I understand it and please correct me 😭) because the socket receive is still synchronous it's not "really" async. It's been a while since I got into the code however.

@dhalbert That would mean modifying the core C code correct?

Sorry for the radio silence all ✌️

dhalbert commented 10 months ago

What @dhalbert is referring to, and can speak much more confidently about is that we can add as much async code in as we want at this level. However, (as I understand it and please correct me 😭) because the socket receive is still synchronous it's not "really" async. It's been a while since I got into the code however.

@dhalbert That would mean modifying the core C code corr

Right, exactly.

dhalbert commented 10 months ago

A lot of my initial changes were a cleanup of the exceptions, etc., and that has been done, though in a different form. The core part are really arequest(), etc.

czei commented 5 months ago

I am confused by "It won't do much until the socket read is also async". Maybe this is older than the current API, but to someone new to CP it looks like the socket class supports asynchronous reads:

https://docs.circuitpython.org/en/latest/shared-bindings/socketpool/index.html

A cursory look at the code on this branch in arequest() seems to indicate the problem is using the http_requests.Response class, which presumably would need to be modified for asynchronous behavior even if the underlying Socket class supports non-blocking reads.

My current project is being prevented from shipping because the entire device locks up while a large JSON file is being read, so I'm motivated to find a short-term solution at the very least.