[Django] #31255: Migration optimizer retains a 1 RemoveField for FK when deleting 2 models
Django
-------------------------------------+-------------------------------------
Reporter: Palisand | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: | Version: 2.2
Migrations |
Severity: Normal | Keywords: migration,optimizer
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
{{{
class ModelA(models.Model):
field1 = models.BooleanField(default=False)
class ModelB(models.Model):
another_field1 = models.TextField(blank=True)
field1 = models.BooleanField(default=False)
related_field = models.ForeignKey(ModelA, on_delete=models.CASCADE)
class ModelC(models.Model):
another_field1 = models.TextField(blank=True)
related_field = models.ForeignKey(ModelA, on_delete=models.CASCADE)
}}}
Removing {{{ModelB}}} and {{{ModelC}}} yields the following migration:
{{{
class Migration(migrations.Migration):
dependencies = [
('analyzer', '0149_modela_modelb_modelc'),
]
operations = [
migrations.RemoveField(
model_name='modelc',
name='related_field',
),
migrations.DeleteModel(
name='ModelB',
),
migrations.DeleteModel(
name='ModelC',
),
]
}}}
It is unclear whether the {{{RemoveField}}} operation is redundant or if a
{{{RemoveField}}} operation for {{{ModelB}}} is missing.
I've confirmed that the {{{RemoveField}}} operation for {{{ModelB}}} is
part of the unoptimized operations set.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Palisand | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Palisand | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:2>
Django
with related fields.
--------------------------------------+------------------------------------
Reporter: Panagis Alisandratos | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Markus Holtermann, Andriy Sokolovskiy, Shai Berger (added)
* version: 2.2 => master
* stage: Unreviewed => Accepted
Comment:
IMO, `RemoveField` is redundant in this case (see #24061 with a related
discussion).
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:3>
Django
with related fields.
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Rohit Jha
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:4>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sanskar Jaiswal):
Upon going through some of the source code, the issue clearly arises
because we are calling `RemoveField` in the `generate_deleted_models`
method in `db/migrations/autodetector.py`
[https://github.com/django/django/blob/f283ffaa84ef0a558eb466b8fc3fae7e6fbb547c/django/db/migrations/autodetector.py#L707].
I think calling this method here is redundant as `DeleteModel` should drop
the table from the database which automatically deletes the related fields
as well. Can someone confirm my hypothesis or correct me if I am steering
in the wrong direction?
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:5>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Sankar it's bit more complicated than that.
If you remove two interelated models (or any model cycle) at the same time
fields have to be removed to break the cycle before hand
Given
{{{#!python
class Foo(models.Model):
bar = models.ForeignKey('Bar')
class Bar(models.Model):
foo = models.ForeignKey(Foo)
}}}
Removing both models and running `makemigrations` must generate a
migration that either drop `Foo.bar` or `Bar.foo` before proceeding with
the model removal. I'm pretty sure we have auto-detector tests for this
case.
The optimizer likely need to be adjusted somehow but I doubt
`generate_deleted_models`'s generation of `RemoveField` is to blame.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:6>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sanskar Jaiswal):
Upon doing some more debugging, I found some peculiar behaviour. I tried
to print `generated_operations` at the end of the function
`generate_deleted_models's `. This is the output: `{'some':
[<RemoveField model_name='book', name='authors_book'>, <DeleteModel
name='Book'>, <RemoveField model_name='pub', name='authors_pub'>,
<DeleteModel name='Pub'>]}`.
The method is adding four operations to the `generated_operations`, but
only three of them show up in the actual migrations file. Am I missing
something here?
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:7>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
> The method is adding four operations to the generated_operations, but
only three of them show up in the actual migrations file. Am I missing
something here?
Yes. The optimizer is likely able to optimize one of the `RemoveField`
into its corresponding `DeleteModel` but not the other one. The reduction
operation takes place in `RemoveField.reduce`.
See #26720 (https://github.com/django/django/pull/7999) and
https://code.djangoproject.com/ticket/24424#comment:35 for more details
about that.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:8>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sanskar Jaiswal):
Unfortunately, I don't think this is the case here. Here is the migration
file that was generated: https://pastebin.com/nSN5HNW8
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:9>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I'm not sure I understand, the migration file you pointed at clearly has 3
operations so the `<RemoveField model_name='book', name='authors_book'>`
got reduced into the `<DeleteModel name='Book'>`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:10>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sanskar Jaiswal):
Forgive me if I come off as a bit unaware, I have just started to work
with the migrations component. I have been going through the code in the
meanwhile, and I am confident that the `reduce()` method or
`MigrationOptimizer`'s `optimize_inner()` method is causing the issue.
Just to be on the same page, the issue here is that while one
`RemoveField` gets reduced into `DeleteModel`, while the other does not,
right?
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:11>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
This is explained in more details in the above links but the crux of the
issue is that `RemoveField` and `DeleteModel` operations don't hold a
reference to ''what'' they are removing so
[https://github.com/django/django/blob/e65fea9292ffdeb9bb76062f6cb2a5ff514ae969/django/db/migrations/operations/models.py#L268-L271
they must assume they could be referring to any model].
In other words, because the `DeleteModel` operation doesn't hold a
reference to the model state it's removing it cannot differentiate between
the cases where the optimization can be performed (this ticket's example)
and when it cannot because there's cycles between models (comment:6).
You likely can get the optimization working for this ticket by changing
`DeleteModel.references_model` to return `False` but
[https://github.com/django/django/blob/e65fea9292ffdeb9bb76062f6cb2a5ff514ae969/tests/migrations/test_autodetector.py#L2029-L2040
that I'll break when for the cases where model cycles are deleted in the
same migration].
If `DeleteModel` operations were created with the set of fields they have
at removal time it would be possible for them to properly determine
whether or not they references each other.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:12>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Here's
[https://github.com/django/django/compare/master...charettes:ticket-31255
a branch pushing the idea above further].
It currently has two failures related to many-to-many field removals
optimizations in the auto-detector but the additional tests in
`test_optimizer` demonstrate that changes work as expected. We should
likely add more tests for the `RemoveField` and `DeleteModel` without
fields(s) tests but I think this should be a good basis if you want to
familiarize yourself with the internals of the optimizer.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:13>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sanskar Jaiswal):
Replying to [comment:13 Simon Charette]:
> Here's
[https://github.com/django/django/compare/master...charettes:ticket-31255
a branch pushing the idea above further].
After going through this, I have built on top of this and further fixed
the ManyToManyField removal test cases and added another test case for
this ticket as well. How should I create the PR, given that I have built
on top of your existing code.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:14>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
> After going through this, I have built on top of this and further fixed
the ManyToManyField removal test cases and added another test case for
this ticket as well
Nice, I think `test_non_cyclical_models_removals` already covers this
ticket's case though; no need to add another one at the auto-detector
level IMO.
> How should I create the PR, given that I have built on top of your
existing code?
Assuming you pushed your work to a branch that includes the above commit
it should be the same process as opening any other PR. I'd leave the two
commits separated so they can be reorganized by the mergers as they deem
the most appropriate.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:15>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sanskar Jaiswal):
While working on my branch, I manually changed all the files to reflect
the changes you made on your branch, and then made some further changes on
my own. I was wondering if there is a way, that the megers recognize that
the PR contains your work too, since if I simply create a PR, there would
be no way of you getting any credit.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:16>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Rohit Jha
Alisandratos |
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I wouldn't worry too much about attribution, mergers can use the Git `Co-
authored-by` feature to give attribution to multiple contributors.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:17>
Django
with related fields.
Reporter: Panagis Alisandratos | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Rohit Jha):
* owner: Rohit Jha => (none)
* status: assigned => new
Comment:
Sanskar Jaiswal: Please feel free to pick this
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:18>
Django
with related fields.
Reporter: Panagis | Owner: Simon
Alisandratos | Charette
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Simon Charette
* status: new => assigned
* has_patch: 0 => 1
Comment:
Submitted a draft PR.
I'd like to try a shot at an alternative patch that changes
`Operation.reduce` signature to `(operation: Operation, app_label: str,
state: ProjectState)` to allow the optimization to be enabled even for
legacy projects with migrations that haven't tracked removed field and
models over time. It would also avoid a ''public'' API change for
`RemoveField` and `DeleteModel`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:19>
Django
with related fields.
-------------------------------------+-------------------------------------
Reporter: Panagis | Owner: Simon
Alisandratos | Charette
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:20>
Django
with related fields.
Reporter: Panagis Alisandratos | Owner: (none)
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Simon Charette => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:21>
Django
with related fields.
--------------------------------------+------------------------------------
Reporter: Panagis Alisandratos | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: migration,optimizer | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31255#comment:22>