Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #25068 -- Added metaclassmaker to resolve metaclass conflicts in migrations state. #5183

Closed
wants to merge 10 commits into from

Conversation

kosz85
Copy link

@kosz85 kosz85 commented Aug 24, 2015

#4968

Now it's faster, or more like it's not slowing down ;)

@timgraham
Copy link
Member

@MarkusH - could you run your benchmarks on this or share how we can do it? I'll happily review the patch for style, etc. if it passes that stage of review.

@MarkusH
Copy link
Member

MarkusH commented Sep 12, 2015

I just use https://github.com/MarkusH/django-migrations-benchmark as some kind of benchmarking tool and uncomment the code in Django I don't want to have affect on the timing.

@kosz85
Copy link
Author

kosz85 commented Sep 17, 2015

@MarkusH I used your benchmark, it seems my current solution doesn't slow down migrations. I have two thoughts after using your benchmark :) Please add __init__.py to your benchmark apps :) It will be easier to use by everyone that need to test it. And second one: this doesn't test my case, so after merging it would be nice to improve it and test also models with multiple metadatas.

Here are results, I ran it when working as I don't have spare computer so it's not best measurement, but I think is enough. As you can see it's almost the same, even better with fix, but difference is caused by IO.

With fix
real    7m7.687s
user    3m4.366s
sys 0m3.366s

real    7m15.488s
user    3m4.684s
sys 0m3.174s

Without fix
real    7m36.196s
user    3m16.878s
sys 0m3.293s

real    7m24.973s
user    3m11.605s
sys 0m3.414s

pass

# Create the same Model without metaclassmaker as you would normally do.
class MetaclassTestModel(six.with_metaclass(ItermidiateMetaClass, models.Model, Mixin)):
Copy link
Member

Choose a reason for hiding this comment

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

Mixin should come before models.Model

Copy link
Author

Choose a reason for hiding this comment

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

I will change it, but you know that this doesn't really matter here? ;) It's just for you.

@MarkusH
Copy link
Member

MarkusH commented Sep 18, 2015

Can you make ItermidiateMetaClass and DummyMetaClass each add an attribute the object, please.

@kosz85
Copy link
Author

kosz85 commented Sep 18, 2015

IntermediateMetaClass shouldn't add additional attrs as this is only "merging" object, as Python is not doing it automatically.
I added attr to DummyMetaClass as suggested.


class DummyMetaClass(type):
dummy_attr = 'dummy'
pass
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this pass anymore.

Copy link
Author

Choose a reason for hiding this comment

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

hehe, you are right :) You want to tire me with this topic to death ;) will fix in minutes.

@MarkusH
Copy link
Member

MarkusH commented Sep 21, 2015

@timgraham if you want to merge, patch looks good to me.

@MarkusH
Copy link
Member

MarkusH commented Sep 22, 2015

Closed in favor of #5334

@MarkusH MarkusH closed this Sep 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants