Skip to content

Fix graphie processing - #604

Merged
marcellamaki merged 4 commits into
developfrom
graphie_fix
May 30, 2025
Merged

Fix graphie processing#604
marcellamaki merged 4 commits into
developfrom
graphie_fix

Conversation

@rtibbles

@rtibbles rtibbles commented May 14, 2025

Copy link
Copy Markdown
Member

Summary

  • Removes opinionated processing from raw Perseus JSON parsing, and instead treats it as plain text looking for images to replace, in the same way the frontend perseus renderer does
  • To still allow the edge case of non-KA perseus JSON with local file references, adds file:// protocol processing to allow references to local file system if needed (adds a test for this)
  • Removes any graphie processing from the BaseQuestion and only puts it into the PerseusQuestion type instead
  • Updates all related tests, including removing tests for now unused methods. Uses pytest parameterize for better tracing of test failures when looping over fixtures - replace unneeded fixtures with module variables

References

Needed to fix learningequality/kolibri-library#20

Reviewer guidance

Have I inadvertently removed some test coverage that might mean that question images are not being properly handled now?

Summary by CodeRabbit

  • New Features

    • Added a new interactive Perseus exercise for labeling a bar graph, including stepwise hints and image support.
  • Refactor

    • Simplified and unified image URL processing in Perseus questions for improved reliability and maintainability.
    • Streamlined test structure by replacing fixtures with parameterized tests and module-level data.
    • Updated _ExerciseGraphieFile handling to support language-specific content retrieval and caching.
    • Enhanced question creation to include language context for Perseus exercises.
  • Bug Fixes

    • Improved handling of local file URLs to ensure compatibility across platforms.
  • Tests

    • Added tests verifying correct processing of local file URLs with file:// protocol.
    • Enhanced test coverage for Perseus question processing with parameterized inputs.
    • Updated tests to use parameterized data, increasing clarity and maintainability.
    • Modified tests to support language-aware _ExerciseGraphieFile instantiation.

@coderabbitai

coderabbitai Bot commented May 14, 2025

Copy link
Copy Markdown

"""

Walkthrough

The changes refactor image URL handling in Perseus question processing, unify and simplify the logic for image replacement using regex, enhance file protocol handling in the disk resource handler, and update tests to use parameterized data instead of fixtures. A new Perseus bar graph labeling exercise JSON is also added for testing.

Changes

File(s) Change Summary
ricecooker/classes/questions.py Refactored image URL processing in PerseusQuestion: replaced recursive field-by-field logic with a single regex substitution over the JSON string, introduced _replace_image method, removed unused methods and imports, updated set_images and set_image methods in BaseQuestion, added ka_language parameter to PerseusQuestion constructor.
ricecooker/utils/pipeline/transfer.py Added _normalize_path to convert file:// URLs to local paths; updated should_handle and handle_file to use normalized paths for file operations.
tests/pipeline/test_transfer.py Added tests verifying DiskResourceHandler correctly processes file:// URLs including Windows UNC paths and ignores non-file protocols; used mocking for network share tests.
tests/test_exercises.py Replaced fixtures with module-level lists and parameterized tests for image and Perseus question handling; removed redundant tests and fixtures; updated assertions for flexibility; added new test data; updated imports and regex usage; removed tests for removed methods.
tests/testcontent/exercises/perseus_question_new_bar_graphs.json Added a new JSON file defining a Perseus exercise for labeling a bar graph, including interactive widgets, hints, and embedded images.
examples/oldexamples/sample_program.py Updated create_question function to pass ka_language argument when instantiating PerseusQuestion.
ricecooker/classes/files.py Modified _ExerciseGraphieFile constructor to require ka_language; updated generate_graphie_file to use ka_language in cache key and download URL logic with fallback; added error handling for missing ka_language.
ricecooker/utils/jsontrees.py Updated add_questions function to pass ka_language when creating PerseusQuestion objects.
tests/conftest.py Updated exercise_graphie_file fixture to instantiate _ExerciseGraphieFile with ka_language argument "en".
tests/test_files.py Updated tests to instantiate _ExerciseGraphieFile with ka_language argument "en" consistently.

Sequence Diagram(s)

sequenceDiagram
    participant PerseusQuestion
    participant RegexSub
    participant _replace_image
    participant FileHandler

    PerseusQuestion->>RegexSub: Apply IMAGE_URL_REGEX to raw_data
    RegexSub->>_replace_image: For each image URL match
    _replace_image->>FileHandler: Check if file exists or create file object with ka_language
    FileHandler-->>_replace_image: Return file reference
    _replace_image-->>RegexSub: Return content storage replacement string
    RegexSub-->>PerseusQuestion: Return processed raw_data with replaced image URLs
Loading

Assessment against linked issues

Objective Addressed Explanation
Download and correctly reference all graphie files in Perseus questions (#20)
"""

[!ANNOUNCEMENT]

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback.
Learn more here.


[!ANNOUNCEMENT]

⚡️ Faster reviews with caching

CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.
Enjoy the performance boost—your workflow just got faster.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8787607 and e81747c.

📒 Files selected for processing (1)
  • ricecooker/classes/files.py (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
ricecooker/classes/files.py (4)
Learnt from: rtibbles
PR: learningequality/ricecooker#604
File: ricecooker/classes/files.py:400-406
Timestamp: 2025-05-14T19:54:13.907Z
Learning: The _ExerciseGraphieFile class in ricecooker/classes/files.py doesn't call super().__init__() in its constructor, which is a known design issue but doesn't cause functional problems in the current implementation.
Learnt from: rtibbles
PR: learningequality/ricecooker#604
File: ricecooker/classes/files.py:400-406
Timestamp: 2025-05-14T19:54:13.907Z
Learning: The _ExerciseGraphieFile class in ricecooker/classes/files.py doesn't call super().__init__ in its constructor, which is a known design issue but doesn't cause functional problems in the current implementation.
Learnt from: rtibbles
PR: learningequality/ricecooker#604
File: ricecooker/classes/files.py:433-439
Timestamp: 2025-05-14T19:51:23.238Z
Learning: Khan Academy Graphie URLs use fixed-length hash identifiers which makes cache key collisions from simple string concatenation extremely unlikely, even when adding language codes to the URLs.
Learnt from: rtibbles
PR: learningequality/ricecooker#604
File: ricecooker/classes/files.py:466-474
Timestamp: 2025-05-14T19:53:24.697Z
Learning: In ricecooker's _ExerciseGraphieFile class, HTTP errors (like 404s) should trigger falling back to English content, while transient errors (timeouts, connection issues) should not trigger fallback as they may be temporary and require retrying.
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Python unit tests (ubuntu-latest, 3.9)
  • GitHub Check: Python unit tests (windows-latest, 3.12)
  • GitHub Check: Python unit tests (windows-latest, 3.11)
  • GitHub Check: Python unit tests (windows-latest, 3.10)
  • GitHub Check: Python unit tests (windows-latest, 3.9)
🔇 Additional comments (6)
ricecooker/classes/files.py (6)

9-9: Good addition for error handling.

Adding the explicit import for HTTPError from requests supports the improved error handling in the localized content fetch logic.


400-404: Improved constructor implementation!

This change properly enhances the _ExerciseGraphieFile class to support localization by:

  1. Adding the required ka_language parameter
  2. Calling super().__init__(**kwargs), which addresses a long-standing design issue
  3. Storing the language code for later use

The call to super().__init__ is particularly important as it ensures proper initialization of inherited attributes.


435-439: Good validation and cache key handling.

The implementation correctly:

  1. Validates that ka_language is specified, preventing potential errors
  2. Incorporates the language code in the cache key for non-English content
  3. Maintains separate caches for different languages (avoiding the use of English cached content for other languages)

457-465: Well-documented localization approach.

The code properly implements Khan Academy's frontend localization approach for graphie files:

  1. The comments clearly explain the path splitting approach and reference the original code
  2. The English case is handled separately (optimized path)
  3. For non-English, the path is correctly modified to insert the language code before the file hash

466-474: Smart fallback mechanism with appropriate error handling.

This implementation correctly:

  1. Attempts to fetch localized content first
  2. Falls back to English content only for specific HTTP errors (like 404s)
  3. Propagates errors for English content (since there's no fallback)
  4. Avoids caching fallback content, ensuring proper localized content is prioritized on subsequent runs

This aligns perfectly with the principle that HTTP errors should trigger fallback while transient errors should not.


480-481: Good conditional caching approach.

Caching is correctly limited to successfully downloaded primary content (not fallback content), which ensures that future runs will try to fetch localized content again rather than relying on cached fallback content.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
ricecooker/utils/pipeline/transfer.py (1)

62-64: should_handle may mis-report on case-insensitive filesystems

os.path.exists is case-sensitive on POSIX but not on Windows. A Windows drive typed with the wrong case (e.g. file:///c:/…) will be normalised to c:\… and exists will still succeed, hiding mismatches that could surface later (e.g. when hashing). Consider using os.path.lexists plus an explicit case check or os.path.realpath to canonicalise paths so the cache key and subsequent hashing are stable across platforms.

tests/pipeline/test_transfer.py (1)

252-266: Guard against spaces & special characters in temporary file:// URIs

When building the file:// URI the test concatenates the raw path, but paths under tests/testcontent can legally contain spaces or #, %, etc. Those characters must be percent-encoded for a standards-compliant URI; while the current code happens to work on most platforms, it will fail once such a filename is added.

-    if platform == "win32":
-        file_path_with_protocol = "file:///" + file_path.replace("\\", "/")
+    from urllib.parse import quote
+    if platform == "win32":
+        file_path_with_protocol = "file:///" + quote(file_path.replace("\\", "/"))
     else:
-        file_path_with_protocol = "file://" + file_path
+        file_path_with_protocol = "file://" + quote(file_path)

Encoding here improves cross-platform fidelity and exercises the new decode logic in _normalize_path.

ricecooker/classes/questions.py (1)

18-19: Regex can over-match and miss single-quoted or escaped URLs

IMAGE_URL_REGEX stops at ") but Perseus JSON occasionally contains single-quoted attributes ('imageUrl':'http://...') or escaped \/ slashes. A more future-proof pattern would be:

IMAGE_URL_REGEX = (
    r"(?P<protocol>web\+graphie|https?|file|data):"
    r"(?P<rawpath>(?:\\.|[^'\"\\\)\s])+)"
)

This excludes whitespace, closing quotes and escaped characters while supporting both ' and " delimiters.

tests/test_exercises.py (1)

209-215: Fix error message formatting in the assertion

The error message concatenation will attempt to add tuples to strings, which will produce an unexpected error format. Use f-strings for more readable error messages.

    assert m.groups() == expected_matches, (
-        "found " + m.groups() + " expected " + expected_matches
+        f"found {m.groups()} expected {expected_matches}"
    )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82625e9 and 4f8f230.

📒 Files selected for processing (5)
  • ricecooker/classes/questions.py (6 hunks)
  • ricecooker/utils/pipeline/transfer.py (3 hunks)
  • tests/pipeline/test_transfer.py (2 hunks)
  • tests/test_exercises.py (2 hunks)
  • tests/testcontent/exercises/perseus_question_new_bar_graphs.json (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ricecooker/utils/pipeline/transfer.py (1)
ricecooker/utils/pipeline/file_handler.py (3)
  • should_handle (34-36)
  • should_handle (275-280)
  • should_handle (294-295)
ricecooker/classes/questions.py (3)
ricecooker/utils/encodings.py (1)
  • get_base64_encoding (8-14)
tests/conftest.py (1)
  • exercise_image_file (852-858)
ricecooker/classes/files.py (11)
  • _ExerciseGraphieFile (395-458)
  • _ExerciseBase64ImageFile (381-385)
  • _ExerciseImageFile (388-392)
  • process_file (167-168)
  • process_file (198-219)
  • process_file (417-429)
  • process_file (468-490)
  • process_file (548-551)
  • get_replacement_str (384-385)
  • get_replacement_str (391-392)
  • get_replacement_str (411-415)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Python unit tests (ubuntu-latest, 3.9)
  • GitHub Check: Python unit tests (windows-latest, 3.12)
  • GitHub Check: Python unit tests (windows-latest, 3.11)
  • GitHub Check: Python unit tests (windows-latest, 3.10)
  • GitHub Check: Python unit tests (windows-latest, 3.9)
🔇 Additional comments (9)
tests/test_exercises.py (3)

243-276: LGTM! Good use of parameterized testing

The conversion from fixtures to parameterized tests makes the test clearer and failures easier to trace. The test correctly verifies that the set_image method performs the expected replacements and saves images properly.


281-334: Well-structured test data setup

Good approach using module-level variables instead of fixtures for test data. The PR objective to fix the issue with new widget types (specifically bar graphs) is addressed with the inclusion of the new test case.


336-361: LGTM! Dynamic assertion for image hashes

The test now dynamically checks the number of expected image hashes rather than using a fixed count, making it more maintainable as new test cases are added. This approach aligns well with the PR objective to improve handling of Perseus JSON.

tests/testcontent/exercises/perseus_question_new_bar_graphs.json (6)

1-8: Answer area configuration is appropriate.

The answerArea object correctly disables all calculators and statistical tables for this labelling exercise. No additional tables appear necessary.


9-20: Hint 1 image mapping looks correct.

The first hint embeds a web+graphie:// URL and provides matching height/width metadata under images. This aligns with the unified regex-based image replacement approach.


33-43: Hint 4 image mapping is sound.

The final hint also embeds a web+graphie:// URL with correct dimension metadata. This will be processed by the same replacement logic.


45-48: Item data version is set appropriately.

Using 0.1 here is consistent with other Perseus fixtures.


49-52: Question content and placeholder look correct.

The main question text and the [[☃ label-image 1]] widget placeholder align with the intended bar-labeling exercise.


53-113: Widget configuration is complete.

The label-image widget defines imageUrl, dimensions, and four markers with the expected answer choices. All fields are present and valid for image replacement and grading.

Comment thread ricecooker/utils/pipeline/transfer.py
Comment thread ricecooker/classes/questions.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
ricecooker/classes/questions.py (1)

292-294: Fixed handling of newlines in web+graphie URLs

This change properly addresses the previous issue where stripped_path was discarded and newline trimming never took effect. By first cleaning the path and then using it in the replacement, web+graphie URLs with stray newlines will now be handled correctly.

🧹 Nitpick comments (1)
ricecooker/classes/questions.py (1)

289-290: Consider using stripped_path consistently

While you're properly handling stripped_path for web+graphie URLs, for other protocols you're still using the original path which might contain whitespace. Consider using the cleaned path consistently for all protocols.

-    full_path = f"{protocol}:{path}"
+    full_path = f"{protocol}:{stripped_path}"
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f8f230 and 50b8a92.

📒 Files selected for processing (5)
  • ricecooker/classes/questions.py (6 hunks)
  • ricecooker/utils/pipeline/transfer.py (3 hunks)
  • tests/pipeline/test_transfer.py (2 hunks)
  • tests/test_exercises.py (2 hunks)
  • tests/testcontent/exercises/perseus_question_new_bar_graphs.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • ricecooker/utils/pipeline/transfer.py
  • tests/testcontent/exercises/perseus_question_new_bar_graphs.json
  • tests/test_exercises.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Python unit tests (ubuntu-latest, 3.9)
  • GitHub Check: Python unit tests (windows-latest, 3.12)
  • GitHub Check: Python unit tests (windows-latest, 3.11)
  • GitHub Check: Python unit tests (windows-latest, 3.10)
  • GitHub Check: Python unit tests (windows-latest, 3.9)
🔇 Additional comments (11)
tests/pipeline/test_transfer.py (6)

4-6: Well-organized import additions for platform-specific testing.

These imports provide the necessary tools for platform detection and mocking file operations, which are essential for testing the new file:// URL handling capabilities without depending on actual file system resources.


254-279: Well-implemented cross-platform file:// protocol test.

This test properly handles platform differences between Windows and Unix-like systems when constructing file:// URLs. It creates a valid path to an existing test file and verifies that the handler correctly processes it, preserving the file extension.


281-312: Comprehensive Windows UNC path handling test with proper mocking.

The test correctly:

  1. Uses pytest.mark.skipif to avoid running on non-Windows platforms
  2. Mocks file operations to avoid requiring actual network shares
  3. Verifies URL normalization from file://server/share/path to proper Windows UNC format (\server\share\path)
  4. Confirms metadata extraction works correctly

This is essential for supporting file references in non-Khan Academy Perseus JSON as mentioned in the PR objectives.


314-337: Good coverage for IP-based UNC paths.

The test extends UNC path support to include IP-based paths (like \192.168.1.100\share), which are common in enterprise environments. The mocking approach avoids dependency on actual network resources while still providing thorough test coverage.


339-371: Thorough testing of Windows file:// URL edge cases.

This test covers important edge cases:

  • Multiple leading slashes
  • URL-encoded spaces and special characters
  • URLs with query parameters or fragments

These cases ensure robust handling of various file:// URL formats that might be encountered in real-world scenarios.


373-386: Important negative test case for non-file protocols.

This test verifies that:

  1. Non-file URLs are left unchanged by the normalization process
  2. The handler correctly refuses to process URLs with other protocols

This prevents the handler from accidentally processing URLs it shouldn't handle, maintaining separation of responsibilities between different resource handlers.

ricecooker/classes/questions.py (5)

18-18: Improved protocol support with comprehensive regex pattern

The new IMAGE_URL_REGEX pattern enhances URL handling by supporting multiple protocols (web+graphie, http, https, file, data), which aligns with the PR objective to add file:// protocol support and treat URLs consistently.


143-143: Simplified API by removing optional parameter

Removing the conditional parsing of HTML streamlines the code path, ensuring consistent handling of all text content.


325-325: Simplified Perseus question processing with regex substitution

The previous approach that recursively traversed the JSON structure has been replaced with a more direct regex-based substitution that treats the JSON as a string. This is a cleaner approach that aligns with the PR objective of removing opinionated processing from raw Perseus JSON parsing.


282-318:

Details

❓ Verification inconclusive

New _replace_image method consolidates image processing logic

This new method centralizes the image replacement logic that was previously scattered across multiple methods, improving maintainability and ensuring consistent handling of image URLs regardless of their source.

However, there's a potential issue: when using protocols other than web+graphie, you're using the original path in full_path (line 289) but not applying the same newline cleanup that you do for web+graphie URLs.

-    full_path = f"{protocol}:{path}"
+    # Create clean path for all protocols, not just web+graphie
+    cleaned_path = stripped_path.lstrip("\\n").rstrip("\\n")
+    full_path = f"{protocol}:{cleaned_path}"

Let's verify if paths with other protocols might contain newlines:


🏁 Script executed:

#!/bin/bash
# Check if any path with http/https/file protocol contains encoded newlines
rg -i "(?:http|https|file):[^\"')]*\\\\n[^\"')]*" --no-filename

Length of output: 3496


Potential newline cleanup gap for non-graphie protocols

Our regex search (rg -i "(?:http|https|file):[^\"')]*\\\\n") surfaced \\n escapes in HTTP and FILE URIs (e.g. warnings and Facebook/YouTube links). While only web+graphie currently strips stray \\n, other protocols may similarly pick up line-break artifacts. To harden this method, consider normalizing stripped_path for all protocols:

-        full_path = f"{protocol}:{path}"
+        # Trim any stray encoded newline characters for any protocol
+        cleaned_path = stripped_path.lstrip("\\n").rstrip("\\n")
+        full_path = f"{protocol}:{cleaned_path}"

Please verify whether image URLs from HTTP/HTTPS/FILE ever contain these \\n sequences in practice, and apply this change if so.


302-303:

Details

❌ Incorrect review comment

Verify base64 encoding check is using the correct value

The get_base64_encoding function is being called with full_path, which includes the protocol prefix. Verify that this function correctly handles input with protocol prefixes, or consider passing only the path portion.


🏁 Script executed:

#!/bin/bash
# Check how get_base64_encoding is implemented and what it expects
rg -A 10 "def get_base64_encoding" --include "*.py"

Length of output: 495


Let’s locate the get_base64_encoding implementation to confirm its expected input. Please run:

This will help verify whether it handles protocol prefixes or expects a plain file path.


🏁 Script executed:

#!/bin/bash
# Search for get_base64_encoding implementation across the repository
rg "def get_base64_encoding" -A 10

Length of output: 709


To confirm how BASE64_REGEX is defined and whether it will match a data-URI string (including the data:image/...;base64, prefix), please run:


🏁 Script executed:

#!/bin/bash
# Display BASE64_REGEX in encodings.py
rg "BASE64_REGEX" -n -A2 -B2 ricecooker/utils/encodings.py

Length of output: 367


No changes needed for base64 check

The get_base64_encoding function uses a regex that explicitly looks for the full data-URI prefix (data:image/...;base64,). Passing full_path directly is correct—it will only return a match when full_path is already a data URI, and safely return None otherwise. There’s no need to strip or modify the input beforehand.

Likely an incorrect or invalid review comment.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
ricecooker/classes/questions.py (1)

282-318: Add documentation for protocol handling

The new method supports multiple protocols but lacks documentation about how each is handled, especially the newly added file:// protocol that's mentioned in the PR objectives.

Consider adding documentation to the method explaining the purpose and how each protocol is handled:

def _replace_image(self, match):
+    """
+    Replace image URLs in Perseus JSON with content storage URLs.
+    
+    Handles multiple protocols:
+    - web+graphie: Removes newlines and converts to https
+    - http/https: Uses the URL directly
+    - file: Handles local file system references
+    - data: Processes data URLs (e.g., base64 encoded images)
+    
+    Args:
+        match: A regex match object with named groups 'protocol' and 'rawpath'
+        
+    Returns:
+        The replacement URL string formatted for content storage
+    """
    protocol = match.group("protocol")
    path = match.group("rawpath")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50b8a92 and a418320.

📒 Files selected for processing (5)
  • ricecooker/classes/questions.py (6 hunks)
  • ricecooker/utils/pipeline/transfer.py (3 hunks)
  • tests/pipeline/test_transfer.py (2 hunks)
  • tests/test_exercises.py (2 hunks)
  • tests/testcontent/exercises/perseus_question_new_bar_graphs.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/pipeline/test_transfer.py
  • tests/testcontent/exercises/perseus_question_new_bar_graphs.json
  • ricecooker/utils/pipeline/transfer.py
  • tests/test_exercises.py
🔇 Additional comments (4)
ricecooker/classes/questions.py (4)

18-18: Support for multiple protocols adds flexibility

The new image URL regex pattern now supports multiple protocols including web+graphie, http/https, file, and data, which aligns with the PR objectives to handle local file references.


290-295: Fixed the newline trimming issue from previous review

The code now properly handles newline characters in web+graphie URLs and correctly assigns the cleaned value to full_path. This fixes the issue raised in the previous review where the trimmed value was discarded.


325-325: Simplified regex-based image URL replacement

Using a single regex replacement instead of recursive JSON traversal makes the code much cleaner and easier to maintain. This aligns with the PR objective of treating Perseus JSON as plain text for image replacement.


134-143:

Details

✅ Verification successful

Parameter removal changes API behavior

The parse_html parameter has been removed from set_images(), making HTML parsing mandatory. Ensure this change doesn't break existing code that relied on the optional behavior.


🏁 Script executed:

#!/bin/bash
# Search for any calls to set_images that explicitly set parse_html=False
rg "set_images\([^,]+,\s*parse_html\s*=\s*False" --type py

Length of output: 60


Here are additional checks to locate any positional or named uses of the removed parameter:


🏁 Script executed:

#!/bin/bash
# List all calls to set_images
rg "set_images\(" -n --type py

# Find any calls passing False as a second (positional) argument
rg "set_images\([^)]*,\s*False" -n --type py

# Find any calls using the named parameter parse_html
rg "set_images\(.*parse_html" -n --type py

Length of output: 505


Internal calls to set_images don’t use the removed parse_html parameter
We searched all calls to set_images() and found only three instances in ricecooker/classes/questions.py, none passing parse_html. Removing this optional flag does not affect existing internal usage.

• ricecooker/classes/questions.py:101
• ricecooker/classes/questions.py:108
• ricecooker/classes/questions.py:125

If set_images() is exposed as a public API, please update its documentation to note that HTML parsing is now mandatory.

Comment thread ricecooker/classes/questions.py
Update Perseus question processing to treat the raw_data as text.
Update exercise tests to use pytest parameterize, and to remove graphie testing from anything but Perseus question.
Remove unused perseus question tests.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (5)
ricecooker/classes/files.py (2)

459-466: Use urllib.parse.urljoin for URL composition

Manual rpartition("/") + string concatenation risks double slashes or missing separators.
urljoin(base_path + "/", f"{self.ka_language}/{file_hash}") is clearer and handles edge-cases (query/fragment) out-of-the-box.


479-481: Consider caching the English variant after fallback

When localisation is missing we skip caching (should_cache = False).
This forces a network round-trip on every subsequent build, even though we successfully downloaded the English JSON.
Writing to cache with a key that encodes lang=en still avoids polluting the locale-specific cache while saving bandwidth.

ricecooker/classes/questions.py (2)

134-154: set_images runs HTML→Markdown conversion every call – heavy for large strings

parse_html() invokes BeautifulSoup+html5lib (slow) even when no <img> tags exist.
A quick guard dramatically reduces overhead:

-if "<img" in text.lower():
-    processed_string = self.parse_html(text)
-else:
-    processed_string = text
+processed_string = self.parse_html(text) if "<img" in text.lower() else text

284-315: _replace_image treats file: URLs as remote paths – loses local-file optimisation

full_path remains file://..., which is later handed to _ExerciseImageFileDownloadFileFilePipeline.
Although _transfer now strips the scheme, we still pay an HTTP-download penalty for local files that already exist.

Minor change:

elif protocol == "file":
    local_path = stripped_path.lstrip("/")   # strip leading `//`
    exercise_image_file = _ExerciseImageFile(local_path)

This skips the transfer layer altogether and avoids double disk I/O.

tests/test_exercises.py (1)

210-215: Assertion message will raise TypeError on failure

"found " + m.groups() + " expected " + expected_matches concatenates str and tuple.
If the assertion ever fails, the message generation itself explodes, masking the real cause.

-    assert m.groups() == expected_matches, (
-        "found " + m.groups() + " expected " + expected_matches
-    )
+    assert m.groups() == expected_matches, (
+        f"found {m.groups()} expected {expected_matches}"
+    )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa32eee and 8787607.

📒 Files selected for processing (7)
  • examples/oldexamples/sample_program.py (1 hunks)
  • ricecooker/classes/files.py (4 hunks)
  • ricecooker/classes/questions.py (7 hunks)
  • ricecooker/utils/jsontrees.py (1 hunks)
  • tests/conftest.py (1 hunks)
  • tests/test_exercises.py (2 hunks)
  • tests/test_files.py (5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
ricecooker/classes/files.py (3)
tests/test_files.py (2)
  • get (52-53)
  • iter_content (1114-1129)
tests/conftest.py (1)
  • iter_content (885-910)
ricecooker/utils/utils.py (1)
  • copy_file_to_storage (75-90)
ricecooker/classes/questions.py (3)
ricecooker/utils/encodings.py (1)
  • get_base64_encoding (8-14)
tests/conftest.py (1)
  • exercise_image_file (852-858)
ricecooker/classes/files.py (11)
  • _ExerciseGraphieFile (396-481)
  • _ExerciseBase64ImageFile (382-386)
  • _ExerciseImageFile (389-393)
  • process_file (168-169)
  • process_file (199-220)
  • process_file (419-431)
  • process_file (491-513)
  • process_file (571-574)
  • get_replacement_str (385-386)
  • get_replacement_str (392-393)
  • get_replacement_str (413-417)
tests/test_exercises.py (3)
tests/test_videos.py (1)
  • _clear_ricecookerfilecache (294-308)
ricecooker/classes/questions.py (5)
  • BaseQuestion (22-229)
  • set_image (176-203)
  • PerseusQuestion (232-326)
  • process_question (95-132)
  • process_question (317-326)
ricecooker/classes/files.py (1)
  • get_filename (103-104)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Python unit tests (ubuntu-latest, 3.9)
  • GitHub Check: Python unit tests (windows-latest, 3.12)
  • GitHub Check: Python unit tests (windows-latest, 3.11)
  • GitHub Check: Python unit tests (windows-latest, 3.10)
  • GitHub Check: Python unit tests (windows-latest, 3.9)
🔇 Additional comments (4)
ricecooker/utils/jsontrees.py (1)

436-436: Adding language support for Perseus Questions

This change adds language context to Perseus questions, enabling proper handling of localized graphie files. The implementation correctly retrieves the language parameter from the question dictionary.

examples/oldexamples/sample_program.py (1)

572-572: Consistent update to pass language parameter to Perseus questions

This change maintains consistency with the PerseusQuestion constructor update, ensuring proper language context is provided for graphie processing in sample code as well.

tests/conftest.py (1)

927-927: Updated test fixture to match new _ExerciseGraphieFile signature

The fixture has been properly updated to provide the required language parameter, ensuring tests continue to work with the refactored graphie processing implementation.

tests/test_files.py (1)

1146-1146: Consistently updated all _ExerciseGraphieFile instantiations with language parameter

All test cases using _ExerciseGraphieFile have been properly updated to include the new required language parameter. This ensures test coverage remains intact after the implementation changes to support language-specific graphie processing.

Also applies to: 1161-1161, 1182-1182, 1192-1192, 1200-1200, 1210-1210, 1215-1215

Comment thread ricecooker/classes/files.py
Comment thread ricecooker/classes/files.py Outdated
Comment thread ricecooker/classes/files.py
Comment thread ricecooker/classes/questions.py
@marcellamaki marcellamaki self-assigned this May 20, 2025

@marcellamaki marcellamaki left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple of questions. (Another perk of the coderabbit review - helps me understand some of the code and decision making. It's nice to come in after that as a reviewer). They're not in and of themselves blocking, but i want to reread this again tomorrow and see if I have any other thoughts or questions, because it's taking me kind of a while to digest it, and then I'll go ahead and approve.

processed_string = self.parse_html(text)
else:
processed_string = text
processed_string = self.parse_html(text)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the change here in a readability sense, but I don't understand the reason for the change

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method used to be used for both !PerseusQuestions and PerseusQuestions. PerseusQuestions should not contain HTML, so we did not parse them as such.

With the changes I made, this method is now only used by !PerseusQuestions so the conditionality was no longer required.

r = config.DOWNLOAD_SESSION.get(json_path_base + "-data.json", stream=True)
r.raise_for_status()
except HTTPError:
if self.ka_language == "en":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, based on the conversation with coderabbit, the reasoning for this is to fallback to english to confirm whether or not the resource exists at all, vs. if it's a temporary connection error that should be retried?

@rtibbles rtibbles May 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTPError indicates there is something wrong with the resource, rather than it just be a temporary connection or timeout issue, so either the resource doesn't exist, is forbidden or whatnot. In that case, we try to get the English version instead, in case it just hasn't been translated yet and that's why we can't access it. Under the rationale that a poorly translated graphic is better than no graphic.

@marcellamaki marcellamaki left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving - I cannot promise there are no problems 😂 and that I missed nothing, but I feel at this point like i can follow along well enough with the code and intention, and it's okay to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Undownloaded graphie files

2 participants