Headerset by the-ivii · Pull Request #563 · learningequality/ricecooker

the-ivii · 2025-01-24T20:56:25Z

Summary

This pull request enhances the User-Agent header generation mechanism in the Ricecooker download utility. The main changes made are:

Introduced a new configure_download_session() function to generate User-Agent headers
Modified the make_request() function to support optional user email identification

References

Fixes: #483

Reviewer guidance

Added test cases to:

Generate requests with various scenarios:
- No email provided
- Custom email supplied
- Different Ricecooker versions

rtibbles

Thanks for this, this is a good start!

This can be done more simply, and more globally by leveraging the information we get from authenticating the user against the Studio server. Please ask any follow up questions about the directions I have suggested.

Also, please make these commits independently of the commits from your other PR.

rtibbles · 2025-01-25T02:23:41Z

ricecooker/managers/tree.py

-                )
-                if response.status_code == 200:
-                    return
+        url_response = config.SESSION.post(config.get_upload_url(), json=data)


Not sure why this code block has been dedented? It wasn't in your other PR.

rtibbles · 2025-01-25T02:30:08Z

ricecooker/utils/downloader.py

    max_retries = 5
    request_headers = DEFAULT_HEADERS
+
+    configure_download_session(sess, user_email)


Note, I think we should be setting this more globally on config.DOWNLOAD_SESSION, not just within the the downloader utility. While it is most acute when we are downloading HTML pages, it is better we do this more broadly.

Once we have authenticated against Studio with the token, we should be able to use the user email from there, rather than requiring it be explicitly passed in.

See here where we receive the username (which is the email address) once we have authenticated with Studio: https://github.com/learningequality/ricecooker/blob/develop/ricecooker/commands.py#L90

Once we have that information in hand, we can do the required DOWNLOAD_SESSION update.

Yes, passing an email manually every time can be tedious. I will work on an approach and make sure that the code is more of Don't Repeat Yourself and improves overall authentication and session management. But, I would like to ask if there any specific requirements or constraints for setting the download session that I need to take care of?

Hi @Divyanshi750 - I'm sorry, I don't understand what isn't clear here - I have outlined which session and needs to be updated, and also shown where in the code this should happen.

the-ivii · 2025-02-06T18:15:08Z

Improved session configuration by adding global email setup after Studio authentication.

Changes made:

Added AuthenticationError class in exceptions.py for better error handling in case the authentication fails
Updated download session configuration to use authenticated user's email when available
Moved session configuration logic to be more globally accessible

This makes the download session configuration more consistent and eliminates the need
to manually pass email for each download operation as requested.Please look into the changes suggested and review the changes made. @rtibbles

rtibbles

In spite of tests being present, the code is full of issues that are evident from a code read through.

This has the appearance of being generated with a lot of LLM assistance, and not sufficient critical reflection of the generated code.

It is also not fixing the issue in the life cycle of a SushiChef class being executed. The header needs to be set on the central DOWNLOAD_SESSION in the config module - and should be done in the place where the user has already been authenticated with a token with the Studio server.

rtibbles · 2025-02-08T20:10:05Z

.gitignore

 cache.sqlite

 chefdata/
+audio_cache.sqlite


This has been added in develop from your other PR - you are still committing the file itself here as well too, so that needs to be removed from the commit history.

rtibbles · 2025-02-08T20:10:15Z

Pipfile

@@ -0,0 +1,11 @@
+[[source]]


This still needs to be removed, as does the file below.

rtibbles · 2025-02-08T20:11:55Z

setup.py

        "console_scripts": [
            "corrections = ricecooker.utils.corrections:correctionsmain",
            "jiro = ricecooker.cli:main",
+            "ricecooker = ricecooker.cli:main",


This doesn't need to be added.

rtibbles · 2025-02-08T20:12:14Z

tests/test_downloader.py

+        )
+
+
+def test_performance_overhead():


What is this test doing?

rtibbles · 2025-02-08T20:13:21Z

tests/test_downloader.py

+baseline_time = timeit.timeit(baseline_request, number=100)
+custom_email_time = timeit.timeit(custom_email_request, number=100)
+
+assert custom_email_time - baseline_time < 0.01


Why are we making assertions in the module level code? This looks like it was generated by an LLM and unthinkingly pasted in here. Please focus only on testing the code you have added around updating the user agent.

rtibbles · 2025-02-08T20:15:04Z

ricecooker/utils/__init__.py

+
+
+def configure_download_session(session, user_email=None):
+    import ricecooker


Why is this being imported twice?

rtibbles · 2025-02-08T20:17:22Z

ricecooker/utils/downloader.py

 from ricecooker.utils.html import download_file
 from ricecooker.utils.html import replace_links
 from ricecooker.utils.zip import create_predictable_zip
+import logging


Why has logging been added here?

rtibbles · 2025-02-08T20:20:00Z

ricecooker/commands.py

+    Studio authentication method that also configures the download session.
+    """
+    try:
+        auth_response = authenticate_studio(username, password)


Why is this function making a recursive call here?

rtibbles · 2025-02-08T20:20:25Z

ricecooker/utils/downloader.py


+
+from . import configure_download_session
+def download_file(url, clear_cookies=False, headers=None, timeout=60, 


What is this new method for?

rtibbles · 2025-02-08T20:21:11Z

ricecooker/utils/downloader.py


 def make_request(
-    url, clear_cookies=False, headers=None, timeout=60, session=None, *args, **kwargs
+    url, clear_cookies=False, headers=None, timeout=60, session=None, user_email=None, *args, **kwargs


I don't think we need to pass this in, we instead copy the user agent from the globally configured DOWNLOAD_SESSION that should have had its user agent headers set properly. So far this PR is not doing this though, so is missing the main point of this issue.

rtibbles · 2025-02-17T19:13:52Z

The majority of comments here are still unaddressed - the main approach needs to be tweaked to focus on the config.DOWNLOAD_SESSION and to be triggered when the tree manager authenticates to Studio.

rtibbles · 2025-03-01T20:25:55Z

This has been force pushed - but there are still conflicts to be resolved, and the principle feedback given is still unaddressed.

the-ivii · 2025-03-09T05:58:40Z

This has been force pushed - but there are still conflicts to be resolved, and the principle feedback given is still unaddressed.

Hi @rtibbles , I was thinking to approach the problem in this way:
Implementing a SessionManager class directly in the tree.py file. It would handle HTTP sessions for downloads and API requests. And then create function to get the current download session or create a new one.
Inside TreeMAnager class, can we implement a function to authenticate to Studio using the provided token and initialize download session.
In downloader.py, a function that would download a file from a URL and save it to a destination or return the contents. And then return the downloaded file.
Let me know if any refinements are needed!

rtibbles · 2025-03-11T20:35:00Z

We just need to update the session on the core config object, and have that update when authentication has happened with Studio, so we have the registered email address. That's all that is needed here.

rtibbles · 2025-04-11T21:42:36Z

This PR has not been updated in over a month, closing.

the-ivii added 3 commits January 23, 2025 19:14

added the insufficient storage exception

f5867ab

added explicit header set

3a0b329

added test cases

e7bcb6f

rtibbles requested changes Jan 25, 2025

View reviewed changes

rtibbles self-assigned this Jan 25, 2025

added audio file to

ffec3c8

rtibbles requested changes Feb 8, 2025

View reviewed changes

the-ivii force-pushed the headerset branch from 6a90db4 to ffec3c8 Compare February 17, 2025 11:06

rtibbles closed this Apr 11, 2025



		def configure_download_session(session, user_email=None):
		import ricecooker



		from . import configure_download_session
		def download_file(url, clear_cookies=False, headers=None, timeout=60,

Uh oh!

Conversation

the-ivii commented Jan 24, 2025

Summary

References

Reviewer guidance

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the-ivii commented Feb 6, 2025

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibbles commented Feb 17, 2025

Uh oh!

rtibbles commented Mar 1, 2025

Uh oh!

the-ivii commented Mar 9, 2025

Uh oh!

rtibbles commented Mar 11, 2025

Uh oh!

rtibbles commented Apr 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants