Skip to content

Conversation

@bbearce
Copy link
Collaborator

@bbearce bbearce commented Aug 3, 2025

django_to_4.2.0

This will be incremental but this is the PR to push django up to 4.2.0 LTS

Asociated issue: #1701

Description

Upgrading django to major version 4.2.0 from 3.0.0

A checklist for hand testing

  • circleci tests only so far

Checklist

  • Code review by me
  • Hand tested by me
  • I'm proud of my work
  • Code review by reviewer
  • Hand tested by reviewer
  • CircleCi tests are passing
  • Ready to merge

@Didayolo Didayolo mentioned this pull request Jul 8, 2025
4 tasks
@ihsaan-ullah
Copy link
Collaborator

I checked the failing tests for test_manual_migration_makes_submissions_from_one_phase_in_another

def test_manual_migration_makes_submissions_from_one_phase_in_another(self):
        self.client.login(username='creator', password='creator')
        # make 5 submissions in phase 1
        for _ in range(5):
            SubmissionFactory(owner=self.creator, phase=self.phase_1, status=Submission.FINISHED, leaderboard=self.leaderboard)
        assert self.phase_1.submissions.count() == 5
        assert self.phase_2.submissions.count() == 0

        # call "migrate" from phase 1 -> 2
        with mock.patch("competitions.tasks.run_submission") as run_submission_mock:
            url = reverse('phases-manually_migrate', kwargs={"pk": self.phase_1.pk})
            resp = self.client.post(url)
            assert resp.status_code == 200
            # Test failing here:
            assert run_submission_mock.call_count == 5

        # check phase 2 has the 5 submissions
        assert self.phase_1.submissions.count() == 5
        # Test failing here:
        assert self.phase_2.submissions.count() == 5

I don't understand some lines and here are some comments:

  • why we are using mock for run_submission? I don't see any use of it in the API or anywhere in the same function. Usually we use mock to mock something but here we are mocking something that we are not using
  • If the mock was not used previously why were the tests passing?
  • manually_migrate API call is not working for some reason. I think the test is not a real test of the API, it is kind of mock testing but something is now wrong with it so the mock is not working. Do you think the test is a real test of the API?

I am still looking into this and will share more details if I find something interesting

@bbearce
Copy link
Collaborator Author

bbearce commented Sep 15, 2025

Ihsaan,

While working on the build errors in circleci I made some changes to django settings files and all non e2e (UI) tests now pass, so the good news is we can "skip" dealing with this perhaps as maybe it was something deeper with the settings file.

I also added these migrations in (tonight) and they are making real simple changes to id fields in the migrations and they were created by django manage.py makemigrations and I after all of this we are down to only e2e to fix.

We can discuss tomorrow but it's getting better...
https://app.circleci.com/pipelines/github/codalab/codabench/2679/workflows/53584440-1d01-4aa7-96c4-af2cc2b52d75/jobs/4539

PS: Ran a search on the code project for mock and a lot of tests seem to use it. I think in this case it is supposed to have made 5 submissions already and then with mock will short circuit the "competitions.tasks.run_submission" end point to fake the 5 new submissions when url = reverse('phases-manually_migrate', kwargs={"pk": self.phase_1.pk}) triggers them to happen after the post request. It will as you say do nothing...but not exactly. Apparently it does this:

The call_count increases automatically every time the mocked function gets called by your actual application code, not by anything you do manually in the test. -ClaudeAI-

So while nothing is done, the mock variable run_submission_mock is tracking how many times it was called, which is what we want if the migration should have made 5 new submissions. At least that is how I understand it.

@ihsaan-ullah
Copy link
Collaborator

ihsaan-ullah commented Sep 23, 2025

It looks like all the e2e are failing because of "Server stopped". According to GPT this comes from channels. We could check if the current version of channels is problematic.

Some feedback from GPT:

Why your tests die with "Server stopped"

channels.testing.LiveServerTestCase in Channels 4+ is more strict:
it spins up an ASGI server in a background thread. If the ASGI app can’t start (bad ASGI_APPLICATION, port conflict, or mis-networking in Docker), the server thread shuts down immediately → pytest shows RuntimeError: Server stopped.

In your docker-compose.selenium.yml, Django exposes 36475, but LiveServerTestCase defaults to 8081.
Since nothing outside the container can hit 8081, Selenium can’t connect, and Django may also fail binding if something already occupies that port in the container → server aborts.

@ihsaan-ullah
Copy link
Collaborator

I have merged develop into django branch and resolved the conflicts. Conflicting files were toml and lock files that i accepted from django branch.

@Didayolo Didayolo mentioned this pull request Oct 7, 2025
3 tasks
@Didayolo
Copy link
Member

Didayolo commented Oct 8, 2025

@ObadaS @wlln @acletournel

Shouldn't we keep this PR for release 1.22.0? 1.21.0 is already quite heavy and late, maybe it is risky to add this on top of it.

@acletournel
Copy link
Collaborator

@Didayolo ok for separating the django upgrade from 1.21. In this case, we will plan the 1.22 directly after.

@ObadaS
Copy link
Collaborator

ObadaS commented Nov 6, 2025

I fixed the issue we had with botocore and HTTPS/HTTP connections. However, we need to update the package to get a real fix.
Related issues :
boto/boto3#4252
boto/boto3#4350

@ObadaS
Copy link
Collaborator

ObadaS commented Dec 11, 2025

Now that #2042 has been merged, we need to rebase this branch so that we can hopefully have passing tests

@ObadaS
Copy link
Collaborator

ObadaS commented Dec 12, 2025

I tried rebasing locally, then ran the makemigrations --merge and migrate commands but they are failing.

Error :

  Applying datasets.0011_alter_data_data_file_alter_data_id_and_more...Traceback (most recent call last):
  File "/app/manage.py", line 15, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 458, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 106, in wrapper
    res = handle_func(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/commands/migrate.py", line 356, in handle
    post_migrate_state = executor.migrate(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/executor.py", line 135, in migrate
    state = self._migrate_all_forwards(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/executor.py", line 167, in _migrate_all_forwards
    state = self.apply_migration(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/executor.py", line 252, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/migration.py", line 118, in apply
    operation.state_forwards(self.app_label, project_state)
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/operations/fields.py", line 219, in state_forwards
    state.alter_field(
  File "/usr/local/lib/python3.10/site-packages/django/db/migrations/state.py", line 272, in alter_field
    fields = self.models[model_key].fields
KeyError: ('datasets', 'datagroup')

Makemigrations --merge :

Merging datasets
  Branch 0011_alter_data_data_file_alter_data_id_and_more
    - Alter field data_file on data
    - Alter field id on data
    - Alter field id on datagroup
  Branch 0013_merge_0011_auto_20250623_1341_0012_delete_datagroup
    - Add field downloads to data
    - Add field is_verified to data
    - Add field license to data
    - Delete model DataGroup
    - Remove field chahub_data_hash from data
    - Remove field chahub_needs_retry from data
    - Remove field chahub_timestamp from data
    - Remove field deleted from data

Merging will only work if the operations printed above do not conflict
with each other (working on different fields or models)
Should these migration branches be merged? [y/N] y

Created new merge migration /app/src/apps/datasets/migrations/0014_merge_20251212_0942.py
Merging tasks
  Branch 0005_alter_solution_id_alter_task_id
    - Alter field id on solution
    - Alter field id on task
  Branch 0005_auto_20250623_1341
    - Remove field chahub_data_hash from solution
    - Remove field chahub_needs_retry from solution
    - Remove field chahub_timestamp from solution
    - Remove field deleted from solution
    - Remove field chahub_data_hash from task
    - Remove field chahub_needs_retry from task
    - Remove field chahub_timestamp from task
    - Remove field deleted from task

Merging will only work if the operations printed above do not conflict
with each other (working on different fields or models)
Should these migration branches be merged? [y/N] y

Created new merge migration /app/src/apps/tasks/migrations/0006_merge_20251212_0942.py
Merging competitions
  Branch 0059_alter_competition_fact_sheet_alter_competition_id_and_more
    - Alter field fact_sheet on competition
    - Alter field id on competition
    - Alter field id on competitioncreationtaskstatus
    - Alter field id on competitiondump
    - Alter field id on competitionparticipant
    - Alter field id on competitionwhitelistemail
    - Alter field id on page
    - Alter field id on phase
    - Alter field id on phasetaskinstance
    - Alter field detailed_result on submission
    - Alter field fact_sheet_answers on submission
    - Alter field id on submission
    - Alter field prediction_result on submission
    - Alter field scoring_result on submission
    - Alter field data_file on submissiondetails
    - Alter field id on submissiondetails
  Branch 0059_auto_20250623_1341
    - Remove field chahub_data_hash from competition
    - Remove field chahub_needs_retry from competition
    - Remove field chahub_timestamp from competition
    - Remove field deleted from competition
    - Remove field chahub_data_hash from competitionparticipant
    - Remove field chahub_needs_retry from competitionparticipant
    - Remove field chahub_timestamp from competitionparticipant
    - Remove field deleted from competitionparticipant
    - Remove field chahub_data_hash from phase
    - Remove field chahub_needs_retry from phase
    - Remove field chahub_timestamp from phase
    - Remove field deleted from phase
    - Remove field chahub_data_hash from submission
    - Remove field chahub_needs_retry from submission
    - Remove field chahub_timestamp from submission
    - Remove field deleted from submission

Merging will only work if the operations printed above do not conflict
with each other (working on different fields or models)
Should these migration branches be merged? [y/N] y

Created new merge migration /app/src/apps/competitions/migrations/0060_merge_20251212_0942.py
Merging profiles
  Branch 0017_alter_deleteduser_id_alter_githubuserinfo_id_and_more
    - Alter field id on deleteduser
    - Alter field id on githubuserinfo
    - Alter field id on membership
    - Alter field id on organization
    - Alter field id on user
  Branch 0019_merge_0017_user_is_banned_0018_auto_20250623_1719
    - Change managers on user
    - Remove field chahub_data_hash from user
    - Remove field chahub_needs_retry from user
    - Remove field chahub_timestamp from user
    - Remove field deleted from user
    - Change managers on user
    - Add field is_banned to user

Merging will only work if the operations printed above do not conflict
with each other (working on different fields or models)
Should these migration branches be merged? [y/N] y

Created new merge migration /app/src/apps/profiles/migrations/0020_merge_20251212_0942.py

I'm not sure if I messed up the rebase, the makemigrations --merge, or if it's a new problem we have to fix

@ObadaS
Copy link
Collaborator

ObadaS commented Dec 12, 2025

Small update, I "deleted" the content of the failing migration file and let the rest of the migrations run.
The website works fine with it and the new playwright tests are passing without issue.
I can also create public datasets without problem.

This is the content of the failing migration:

# Generated by Django 4.2.23 on 2025-09-08 12:32

from django.db import migrations, models
import utils.data
import utils.storage


class Migration(migrations.Migration):

        dependencies = [
            ("datasets", "0010_auto_20250218_1100"),
        ]

        operations = [
            migrations.AlterField(
                model_name="data",
                name="data_file",
                field=models.FileField(
                    blank=True,
                    null=True,
                    storage=utils.storage.PrivateStorageClass(),
                    upload_to=utils.data.PathWrapper("dataset"),
                ),
            ),
            migrations.AlterField(
                model_name="data",
                name="id",
                field=models.BigAutoField(
                    auto_created=True, primary_key=True, serialize=False, verbose_name="ID"
                ),
            ),
            migrations.AlterField(
                model_name="datagroup",
                name="id",
                field=models.BigAutoField(
                    auto_created=True, primary_key=True, serialize=False, verbose_name="ID"
                ),
            ),
        ]

I changed it to be

# Generated by Django 4.2.23 on 2025-09-08 12:32

from django.db import migrations, models
import utils.data
import utils.storage


class Migration(migrations.Migration):
        pass

I doubt this is what's supposed to be done in the first place but at least I was able to launch the new e2e tests on the django4 branch locally

@ObadaS
Copy link
Collaborator

ObadaS commented Dec 16, 2025

FAILED src/apps/api/tests/test_competitions.py::PhaseMigrationTests::test_manual_migration_makes_submissions_from_one_phase_in_another - AssertionError: assert 0 == 5
FAILED src/apps/api/tests/test_competitions.py::PhaseMigrationTests::test_manual_migration_makes_submissions_out_of_only_parents_not_children - AssertionError: assert 0 == 1
FAILED src/apps/api/tests/test_submissions.py::TaskSelectionTests::test_can_re_run_submissions_with_multiple_tasks - AttributeError: 'NoneType' object has no attribute 'pk'
FAILED src/apps/api/tests/test_submissions.py::TaskSelectionTests::test_can_select_tasks_when_making_submissions - assert [] == [229, 230]
FAILED src/apps/competitions/tests/test_submissions.py::MultipleTasksPerPhaseTests::test_children_always_created_in_the_same_order - AssertionError: assert 1 == 2
FAILED src/apps/competitions/tests/test_submissions.py::MultipleTasksPerPhaseTests::test_making_submission_creates_parent_sub_and_additional_sub_per_task - AssertionError: assert 1 == 2

New unit test errors after migrations

It seems like the tests were failing locally because we have to use the .env_circleci file instead of the .env_sample before running the tests

@ObadaS
Copy link
Collaborator

ObadaS commented Dec 18, 2025

Just discovered that Celery is not unzipping and sending submissions anymore. This is now done by Django for some reason.

We need to figure out why this happens and probably change it back to be handled by Celery

This is because of this env variable in the DJANGO_SETTINGS_MODULE=settings.test settings from .env_circleci that links to the test.py settings file for django, which as this:

CELERY_TASK_ALWAYS_EAGER = True

If we copy the normal .env the problem does not appear.

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.

6 participants