Conversation
…resources, update edx-lint to 6.0.0 Agent-Logs-Url: https://github.com/mitodl/edx-sga/sessions/2e4e0934-d461-4d3d-8bb4-c245cfe8a691 Co-authored-by: asadali145 <52656433+asadali145@users.noreply.github.com>
db5b182 to
a3639b8
Compare
There was a problem hiding this comment.
Pull request overview
Updates edx_sga to run cleanly on Python 3.12 by removing reliance on pkg_resources and aligning tooling/config with newer Python/Django environments.
Changes:
- Replace
pkg_resourcesresource loading inedx_sga/sga.pywith stdlibimportlib.resources. - Upgrade
edx-lintand regeneratepylintrcfor compatibility. - Update test and integration-test scaffolding for newer Django storage configuration and edx-platform config layout.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
edx_sga/sga.py |
Switch resource loading from pkg_resources to importlib.resources. |
test_requirements.txt |
Bump edx-lint to avoid pkg_resources dependency in lint tooling. |
pylintrc |
Update generated header/hash for edx-lint 6.0.0. |
edx_sga/test_settings.py |
Move from DEFAULT_FILE_STORAGE to Django 4.2+ STORAGES setting. |
edx_sga/tests/common.py |
Update temp storage test mixin to use STORAGES. |
edx_sga/tests/integration_tests.py |
Create a non-staff user with an associated UserProfile for runtime prep. |
run_edx_integration_tests.sh |
Copy setup.cfg or fall back to pyproject.toml from edx-platform for integration runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
edx_sga/sga.py
Outdated
| data = files(__name__).joinpath(path).read_bytes() | ||
| return data.decode("utf8") |
There was a problem hiding this comment.
importlib.resources.files() expects a package (or package name). In this module, __name__ is edx_sga.sga (not a package), so files(__name__) can raise “is not a package” at runtime. Use files(__package__) or files('edx_sga') (or pass the actual edx_sga package module) so resources resolve correctly.
edx_sga/sga.py
Outdated
| resource_content = files(__name__).joinpath(resource_path).read_bytes() | ||
| return str(resource_content.decode("utf8")) |
There was a problem hiding this comment.
Same issue as _resource(): files(__name__) is using the module name (edx_sga.sga) rather than the package, which can break resource loading. Switch to files(__package__) / files('edx_sga') for consistent behavior across environments (including zipped distributions).
| # Copy setup.cfg if it exists, otherwise try pyproject.toml | ||
| if [ -f /openedx/edx-platform/setup.cfg ]; then | ||
| cp /openedx/edx-platform/setup.cfg . | ||
| elif [ -f /openedx/edx-platform/pyproject.toml ]; then | ||
| cp /openedx/edx-platform/pyproject.toml . | ||
| else | ||
| echo "Neither setup.cfg nor pyproject.toml found, skipping." | ||
| fi |
There was a problem hiding this comment.
PR description lists changes to edx_sga/sga.py, test_requirements.txt, and pylintrc, but this PR also updates integration test script + multiple test/settings files (e.g., run_edx_integration_tests.sh, edx_sga/test_settings.py, edx_sga/tests/common.py, edx_sga/tests/integration_tests.py). Please update the description to reflect these additional behavioral changes so reviewers know what to focus on.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
The PR description has been updated to document all changed files:
edx_sga/test_settings.py: migratedDEFAULT_FILE_STORAGE→STORAGESdict (Django 4.2+ deprecation)edx_sga/tests/common.py: updated test setup/teardown to save and restoreSTORAGES["default"]instead of the removedDEFAULT_FILE_STORAGEsettingedx_sga/tests/integration_tests.py: fixed user creation in staff/non-staff test helper to create a properUser+UserProfilepairrun_edx_integration_tests.sh: added fallback to copypyproject.tomlwhensetup.cfgis absent from edx-platform
…oading Agent-Logs-Url: https://github.com/mitodl/edx-sga/sessions/45039174-a75e-49f7-904b-03920ef8b1ee Co-authored-by: asadali145 <52656433+asadali145@users.noreply.github.com>
|
@arslanashraf7 This should be ready for a quick look. I have incorporated the AI review and also tested it. |
pkg_resourcesis no longer available by default in Python 3.12 environments, causingModuleNotFoundErroron import ofedx_sga.sgaand cascading failures across all 27 tests in CI.What are the relevant tickets?
N/A
What's this PR do?
edx_sga/sga.py: Replaceimport pkg_resourceswithfrom importlib.resources import files(stdlib, Python 3.9+). Update_resource()andload_resource()to usefiles(__package__).joinpath(path).read_bytes()— using__package__(edx_sga) rather than__name__(edx_sga.sga) ensures resources resolve against the package directory correctly, including in zipped distributions.test_requirements.txt: Upgradeedx-lintfrom5.3.6→6.0.0— old version usedpkg_resourcesinternally in its pylint plugin, causing a second failure point.pylintrc: Regenerated viaedx_lint write pylintrcfor edx-lint 6.0.0 compatibility.edx_sga/test_settings.py: Migrated the deprecatedDEFAULT_FILE_STORAGEsetting to theSTORAGESdictionary format required by Django 4.2+.edx_sga/tests/common.py: Updated test setup/teardown helpers to save and restoreSTORAGES["default"]instead of the removedDEFAULT_FILE_STORAGEsetting.edx_sga/tests/integration_tests.py: Fixed user creation in the staff/non-staff parameterized test helper to create a properUser+UserProfilepair instead of a bareUser.objects.create().run_edx_integration_tests.sh: Added fallback logic to copypyproject.tomlfrom edx-platform whensetup.cfgis absent (newer edx-platform releases no longer shipsetup.cfg).How should this be manually tested?
Run
tox -e py312-django42andtox -e py312-django52— both should pass all 64 tests with no pylint errors.Where should the reviewer start?
edx_sga/sga.pylines 9–18 (import change) and the_resource/load_resourcefunctions near the bottom of the file. Also reviewedx_sga/test_settings.pyandedx_sga/tests/common.pyfor theSTORAGESmigration, andrun_edx_integration_tests.shfor thesetup.cfg/pyproject.tomlfallback.Any background context you want to provide?
pkg_resourcesis provided bysetuptools, which is no longer bundled by default in Python 3.12 venvs.importlib.resourcesis the stdlib replacement and has been available since Python 3.9.files(__package__)is used instead offiles(__name__)because__name__refers to the moduleedx_sga.sgarather than theedx_sgapackage where static resources reside.The
DEFAULT_FILE_STORAGEsetting was deprecated in Django 4.2 in favour of theSTORAGESdict; test scaffolding has been updated accordingly. The integration test runner now gracefully handles edx-platform repositories that have migrated fromsetup.cfgtopyproject.toml.Screenshots (if appropriate)
N/A
What GIF best describes this PR or how it makes you feel?
N/A
Original prompt
Fix failing GitHub Actions tests in mitodl/edx-sga
Tests are currently failing in the GitHub Actions CI workflow for the
mitodl/edx-sgarepository. Please investigate the failing tests, identify the root causes, and fix them.Steps to follow
masterbranch to identify which tests are failing and what errors are reported.tox.ini,setup.py,requirements*.txt) to understand the failure context.Known issues to look for (based on prior analysis)
ModuleNotFoundError: No module named 'pkg_resources'on Python 3.12 — caused byimport pkg_resourcesinedx_sga/sga.py. Fix by replacingpkg_resourcesusage withimportlib.resources.files(available in Python 3.9+ stdlib).AttributeError: module 'edx_sga' has no attribute 'sga'— caused by the above import failure leavingedx_sgapartially initialized.Expected fix for
edx_sga/sga.pyimport pkg_resourcesfrom importlib.resources import files_resource():load_resource():Also check for any other test failures not covered above and fix those as well.
The following is the prior conversation context from the user's chat exploration (may be truncated):
User: Please find a solution for this failing job. Use the logs, job definition, and any referenced files where the failure occurred. Keep your response focused on the solution and include code suggestions when appropriate.
Assistant: Let me fetch the job logs to investigate the failure.
Assistant: There are two distinct root causes for the 27 test failures. Here's the breakdown and fixes:
Root Cause 1:
ModuleNotFoundError: No module named 'pkg_resources'pkg_resourceswas removed from Python 3.12's standard library. It's provided bysetuptools, butsetuptoolsis no longer bundled by default in newer Python environments.Affected tests:
test_ctor,test_max_score,test_save_sga,test_upload_allowed,test_clear_student_state, etc.The issue is in
edx_sga/sga.pyline 17:...and its usages at lines 1042–1051.
Fix A — Replace
pkg_resourceswithimportlib.resources(recommended)Fix B — Alternatively, pin
setuptoolsas a dependencyAdd it to
setup.py'sinstall_requires(orrequirements.txt):Fix A is preferred since
pkg_resourcesis deprecated for resource loading.Root Cause 2:
AttributeError: module 'edx_sga' has no attribute 'sga'The test's
fake_importinsetUpinterceptsImportErrors but doesn't handle the case whereedx_sga.sgafails to import due to thepkg_resourceserror — leaving theedx_sgamodule in a partially-initialized state. The subsequentfrom edx_sga.sga import StaffGradedAssignmentXBlockinmake_xblockthen findsedx_sgawithout asgaattribute.Affected tests:
test_download_annotated,test_student_view,test_staff_download, etc.This is a cascading failure from Root Cause 1. Fixing
pkg_resources(above) will resolve these as well, sinceedx_sga/sga.pywill then import cleanly.Summary
pkg_resourceswithimportlib.resourcesinsga.pyModuleNotFoundErroron Python 3.12edx_sga.sgaimports cleanlyAttributeError: module 'edx_sga' has no attribute 'sga'failuresApplying Fix A alone should resolve all 27 failing tests.
User: Create a PR with the Fix A
Assistant: We're waiting for your response to the tool call confirmation dialog to continue.
User: @copilot A...
This pull request was created from Copilot chat.