[Django Upgrade] [ENG-3947] Fix django debug toolbar and profiling tools for Django 3 #9987

Johnetordoff · 2022-07-21T14:45:53Z

Purpose

The DJDTB is breaking when updating to Django 3 and though the bugs aren't particularly hard to fix this PR removes the DJDTB and other local profiling tools which could produce other issues in the future.

https://www.notion.so/cos/8-Django-Debug-toolbar-namespace-problem-0142b5d4dced4bbd9ce8d311d6935e6a

Changes

Moves local requirements to local.txt
isolates CI only requirments to dev.txt

QA Notes

Please make verification statements inspired by your code and what your code touches.

Verify
Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

api/base/urls.py

requirements/dev.txt

cslzchen

As discussed in flow, people may still use these tools. Let's keep it as for "pre-upgrade" fixes. We can remove these after the upgrade if it take too much effort to fix.

Johnetordoff · 2022-08-11T12:31:34Z

tasks/__init__.py

-                echo=True
-            )
+            install_from_txt(ctx, os.path.join(HERE, 'requirements', 'dev.txt'))
+        elif base:  # then base requirements


I changed this to an elif because these .txt all contain a -r ../requirements.txt so they incorporate those reqs anyway.

cslzchen

Looks good as discussed. I will make a few updates and push back before merge. See comments for details.

cslzchen · 2022-08-16T14:56:10Z

.gitignore

@@ -126,6 +126,7 @@ api/static/vendor
 # Local settings files
 local.py
 local.json
+requirements/debug.txt


I would just have debug.txt instead of having a -dist version, which I will update and push back.

cslzchen · 2022-08-16T15:00:38Z

requirements/debug-dist.txt

@@ -0,0 +1,17 @@
+-r ../requirements.txt
+
+# Requirements that are used locally only for a good developer experience


Just to mention the difference, missing two removed packages. I am OK removing django-sslserver but why removing django-silk? I will add it back in my update.

django-sslserver==0.19 django-silk==2.0.0

cslzchen · 2022-08-16T15:04:12Z

tasks/__init__.py

@@ -19,7 +19,7 @@


 try:
-    from tasks import local  # noqa
+    from tasks import local as local_dist # noqa


👍 I see. We import here just to check file existence without using it. That's why I am not seeing any usage diff.

cslzchen · 2022-08-16T15:07:20Z

tasks/__init__.py

+def install_from_txt(ctx, req_path):
+    ctx.run(
+        pip_install(req_path),
+        echo=True
+    )


👍 for reducing dup code, I will add docstr

cslzchen · 2022-08-16T15:14:36Z

tasks/__init__.py

+        if debug:  # then local debugging tools
+            install_from_txt(ctx, os.path.join(HERE, 'requirements', 'debug.txt'))


Wondering about the orders of dev, base and debug. debug is another level after dev thus needs dev. I think updating the -r option in debug.txt to point dev.txt will solve this.

cslzchen · 2022-08-16T15:15:49Z

admin/base/settings/defaults.py

-
-if DEBUG:
-    INSTALLED_APPS += ('debug_toolbar', 'nplusone.ext.django',)
-    MIDDLEWARE += ('debug_toolbar.middleware.DebugToolbarMiddleware', 'nplusone.ext.django.NPlusOneMiddleware',)
-    DEBUG_TOOLBAR_CONFIG = {
-        'SHOW_TOOLBAR_CALLBACK': lambda _: True,
-        'DISABLE_PANELS': {
-            'debug_toolbar.panels.templates.TemplatesPanel',
-            'debug_toolbar.panels.redirects.RedirectsPanel'
-        }
-    }
-
-# If set to True, automated tests with extra queries will fail.
-NPLUSONE_RAISE = False


I believe our plan is to keep these.

cslzchen · 2022-08-16T15:16:05Z

admin/base/urls.py

-if DEBUG:
-    import debug_toolbar
-
-    urlpatterns += [
-        url(r'^__debug__/', include(debug_toolbar.urls)),
-    ]
-


cslzchen · 2022-08-16T15:16:20Z

api/base/settings/defaults.py

-# If set to True, automated tests with extra queries will fail.
-NPLUSONE_RAISE = False
-


cslzchen · 2022-08-16T15:16:43Z

api/base/settings/local-dist.py

 if DEBUG:
-    INSTALLED_APPS += ('debug_toolbar', 'nplusone.ext.django',)
-    MIDDLEWARE += (
-        'debug_toolbar.middleware.DebugToolbarMiddleware',
-        'nplusone.ext.django.NPlusOneMiddleware',
-    )
-    DEBUG_TOOLBAR_CONFIG = {
-        'SHOW_TOOLBAR_CALLBACK': lambda _: True,
-    }
    ALLOWED_HOSTS.append('localhost')

-    # django-silk
-    INSTALLED_APPS += ('silk',)
-    MIDDLEWARE += (
-        'django.contrib.sessions.middleware.SessionMiddleware',
-        'silk.middleware.SilkyMiddleware',
-    )
-


cslzchen · 2022-08-16T15:17:01Z

api/base/urls.py

-# Add django-silk URLs if it's in INSTALLED_APPS
-if 'silk' in settings.INSTALLED_APPS:
-    urlpatterns += [
-        url(r'^silk/', include('silk.urls', namespace='silk')),
-    ]
-
-if settings.DEBUG:
-    import debug_toolbar
-
-    urlpatterns += [
-        url(r'^__debug__/', include(debug_toolbar.urls)),
-    ]
-
-


cslzchen · 2022-08-16T18:05:37Z

@Johnetordoff the test failure tells us either that our GitHub Action tests have settings.DEBUG set to true ~~or that somehow requirements cannot be conditionally required/chosen by using an if statement~~

Update: confirmed local-travis.py has DEBUG_MODE = True.

…neric behavior

cslzchen · 2022-08-16T20:26:48Z

A minor concern that I forgot to mention, though I doubt it is actually an issue: DEBUG might have been turned on for a reason for CI (instead of just a copy from local settings). We shouldn't just turn it off to pass CI tests.

cslzchen · 2022-09-01T15:56:19Z

PR closed since #10049 fixes the issue with a few troubling dev/local requirements.

Johnetordoff force-pushed the remove-django-debug-toolbar branch from 1c255aa to a98cff1 Compare July 21, 2022 14:59

cslzchen force-pushed the feature/django_upgrade branch from 65b0360 to 250b832 Compare July 27, 2022 19:20

Johnetordoff mentioned this pull request Aug 4, 2022

Django 3 semi-stable #10007

Closed

14 tasks

Johnetordoff force-pushed the remove-django-debug-toolbar branch from 4e84337 to 7c845bb Compare August 4, 2022 20:30

Johnetordoff marked this pull request as ready for review August 4, 2022 20:49

cslzchen reviewed Aug 5, 2022

View reviewed changes

api/base/urls.py Outdated Show resolved Hide resolved

cslzchen reviewed Aug 5, 2022

View reviewed changes

api/base/urls.py Outdated Show resolved Hide resolved

cslzchen reviewed Aug 5, 2022

View reviewed changes

requirements/dev.txt Show resolved Hide resolved

cslzchen reviewed Aug 5, 2022

View reviewed changes

requirements/dev.txt Outdated Show resolved Hide resolved

Johnetordoff force-pushed the remove-django-debug-toolbar branch 8 times, most recently from 8907783 to 0f33c1f Compare August 9, 2022 15:55

cslzchen reviewed Aug 10, 2022

View reviewed changes

cslzchen changed the title ~~Remove django debug toolbar and profiling tools for Django 3~~ [Django Upgrade] [Pending] Remove django debug toolbar and profiling tools for Django 3 Aug 10, 2022

Johnetordoff force-pushed the remove-django-debug-toolbar branch 6 times, most recently from 868882d to b15db2e Compare August 10, 2022 19:43

cslzchen self-requested a review August 11, 2022 04:01

cslzchen changed the title ~~[Django Upgrade] [Pending] Remove django debug toolbar and profiling tools for Django 3~~ [Django Upgrade] [ENG-3947] Remove django debug toolbar and profiling tools for Django 3 Aug 11, 2022

Johnetordoff commented Aug 11, 2022

View reviewed changes

Johnetordoff force-pushed the remove-django-debug-toolbar branch from b15db2e to aff3e8a Compare August 11, 2022 12:37

cslzchen requested changes Aug 16, 2022

View reviewed changes

Johnetordoff changed the title ~~[Django Upgrade] [ENG-3947] Remove django debug toolbar and profiling tools for Django 3~~ [Django Upgrade] [ENG-3947] Fix django debug toolbar and profiling tools for Django 3 Aug 16, 2022

Update django debug requirments

a222153

Johnetordoff force-pushed the remove-django-debug-toolbar branch from 415be8a to a222153 Compare August 16, 2022 17:27

turn off debug requirements for CI, refactor travis-local for new ge…

b0c3e27

…neric behavior

Johnetordoff force-pushed the remove-django-debug-toolbar branch from e17a48d to b0c3e27 Compare August 16, 2022 19:38

cslzchen mentioned this pull request Sep 1, 2022

[Django Upgrade] [ENG-3999] Django3 Upgrade Base #10049

Merged

cslzchen closed this Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Django Upgrade] [ENG-3947] Fix django debug toolbar and profiling tools for Django 3 #9987

[Django Upgrade] [ENG-3947] Fix django debug toolbar and profiling tools for Django 3 #9987

Johnetordoff commented Jul 21, 2022 •

edited

Loading

cslzchen left a comment

Johnetordoff Aug 11, 2022

cslzchen left a comment

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen Aug 16, 2022

cslzchen commented Aug 16, 2022 •

edited

Loading

cslzchen commented Aug 16, 2022

cslzchen commented Sep 1, 2022

		@@ -0,0 +1,17 @@
		-r ../requirements.txt

		# Requirements that are used locally only for a good developer experience

		if debug: # then local debugging tools
		install_from_txt(ctx, os.path.join(HERE, 'requirements', 'debug.txt'))

		# If set to True, automated tests with extra queries will fail.
		NPLUSONE_RAISE = False

[Django Upgrade] [ENG-3947] Fix django debug toolbar and profiling tools for Django 3 #9987

[Django Upgrade] [ENG-3947] Fix django debug toolbar and profiling tools for Django 3 #9987

Conversation

Johnetordoff commented Jul 21, 2022 • edited Loading

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

cslzchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen commented Aug 16, 2022 • edited Loading

cslzchen commented Aug 16, 2022

cslzchen commented Sep 1, 2022

Johnetordoff commented Jul 21, 2022 •

edited

Loading

cslzchen commented Aug 16, 2022 •

edited

Loading