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.

Sign up for GitHub

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

Jump to bottom

Replace existing usages of requests with NetworkClient #11018

Open
Tracked by #10186
bjester opened this issue Jul 26, 2023 · 7 comments · May be fixed by #12096
Open
Tracked by #10186

Replace existing usages of requests with NetworkClient #11018

bjester opened this issue Jul 26, 2023 · 7 comments · May be fixed by #12096
Assignees
@thesujai
Labels
DEV: backend Python, databases, networking, filesystem... help wanted Open source contributors welcome TAG: tech update / debt Change not visible to user

Comments

@bjester
Copy link
Member

bjester commented Jul 26, 2023 •
edited

Background

We've made our NetworkClient utility more flexible and incorporated sensible default timeouts and a custom user agent. It's also now smarter when given a NetworkLocation.

Goals

There are several usages of requests that bypass the custom behavior of NetworkClient. Ideally they should be refactored to use the NetworkClient instead. Some additional functionality and flexibility may be required to handle all other use cases of requests.
@bjester bjester mentioned this issue Jul 26, 2023
Open
@bjester bjester changed the title Replace existing usages of requests with NetworkClient to ensure proper timeouts; it's also now smarter when given a NetworkLocation Replace existing usages of requests with NetworkClient Jul 26, 2023
@bjester bjester added the TAG: tech update / debt Change not visible to user label Jul 26, 2023
@bjester bjester mentioned this issue Jul 26, 2023
Merged
9 tasks
@rtibbles rtibbles added help wanted Open source contributors welcome DEV: backend Python, databases, networking, filesystem... labels Sep 15, 2023
@andreamisuraca
Copy link
Contributor

Hi @rtibbles @bjester, can I give it a try with this task? Could you please provide more info?

@rtibbles
Copy link
Member

Yes, that would be great.

The first thing to do would be to look for places in the codebase where the requests module is used directly to make requests. Making a TODO list of them in a comment here would be a good first step, and we can weigh in, in case any are not needed to be migrated.

The next step would then to start replacing them with usage of the NetworkClient class. The best example of using the NetworkClient class for requests can be found here:

kolibri/kolibri/core/discovery/api.py

Line 91 in e67339e

with NetworkClient.build_from_network_location(peer_device) as client:

@andreamisuraca
Copy link
Contributor

The requests module should be replaced in the following Kolibri modules:

  • core
  • core/analytics
  • core/auth
  • core/auth/management
  • core/auth/management/commands
  • core/auth/utils
  • core/content
  • core/content/utils
  • core/utils
  • plugins/setup_wizard
  • plugins/user_profile

The requests module will still be used in utils (Kolibri modules shouldn't be imported there) and in the build_tools. Is that right @rtibbles?
Should the exceptions be replaced as well? Or just the HTTP requests?

@MisRob
Copy link
Member

MisRob commented Dec 15, 2023

The requests module should be replaced in the following Kolibri modules

@andreamisuraca I'm sorry, we missed this. We'll reach out.

@rtibbles
Copy link
Member

Hi @andreamisuraca - apologies for missing this! We definitely don't need to replace in the build_tools.

One of the main motivations for using the NetworkClient is to ensure we have a user agent that shows this is Kolibri - we may want to look into how we can use it in utils as well - but we can do that as a follow up issue, as it may require some additional work to extricate from other code.

Exceptions can be kept the same, as it is still using requests under the hood - we're just wrapping it to ensure consistency!

@thesujai
Copy link
Contributor

May I work on this?

@MisRob
Copy link
Member

MisRob commented Apr 19, 2024

Hi @thesujai, yes, thank you

@MisRob MisRob assigned thesujai and unassigned andreamisuraca Apr 19, 2024
@thesujai thesujai linked a pull request Apr 19, 2024 that will close this issue
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thesujai thesujai

Labels
DEV: backend Python, databases, networking, filesystem... help wanted Open source contributors welcome TAG: tech update / debt Change not visible to user
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replacing requests with NetworkClient thesujai/kolibri
5 participants
@bjester @rtibbles @MisRob @andreamisuraca @thesujai