--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2bdf074c-5e21-48cf-a93c-ba19f7744a89%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/dbc29625-3af4-4b73-9b60-78d269352d52%40googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/a5d0f33d-882f-4e02-9191-281594ff5522%40googlegroups.com.
I had assumed this was still a work in progress because there are missing tests and some documentation. The build is passing but the unittest coverage for the new modules seems low or at least not up to the standards I expect for Django contributions. The same for the daphne and asgiref packages which are new requirements. In previous discussions there were talks about performance benchmarks to track potential regressions before this would be accepted similar to the template-based widget rendering change. I don't see any references here or in this work. What is the status of those?
As noted in the PR there is at least one new setting, CHANNEL_SESSION_ENGINE, which is lacking documentation.
The notes on deployment for "Running ASGI under WSGI" don't give any motivation why you would want to do this given that it doesn't support websockets in this case. Is there any advantages? Disadvantages?
There are tests for the routing and request handling but there are no unittests that I can see for the auth/session handling,
Channel/Message/Group/ChannelLayerManager/custom JSON encoder classes and their usage,
or the worker/runworker code.
The changes to runserver aren't tested.
The new test utilities aren't tested either (yes that's a thing).
All new code should have tests. Isn't that Django's current standard?
asgiref has one "basic" test around WSGI handling which the usage is documented in this change. I don't think anyone would dare say that covers potential edge cases in WSGI/HTTP handling. Not test related, the status code map is also missing many HTTP status codes.
I'm not in favor of on leeway for external dependencies for the reason you note: they are in the install_requires. Django has avoided having required dependencies and I don't think it sets a good precedent to have the first set be those which don't meet the quality standards expected by the community. That is they don't have sufficient docs/tests to be include in Django itself. It isn't clear why these are required if you are claiming that they aren't required to run. If these we're in the install_requires then I would withdraw the objections about them with regard to this change in Django.
On Wednesday, May 4, 2016 at 10:26:22 AM UTC-4, Andrew Godwin wrote:On Wed, May 4, 2016 at 6:15 AM, Mark Lavin <markd...@gmail.com> wrote:I had assumed this was still a work in progress because there are missing tests and some documentation. The build is passing but the unittest coverage for the new modules seems low or at least not up to the standards I expect for Django contributions. The same for the daphne and asgiref packages which are new requirements. In previous discussions there were talks about performance benchmarks to track potential regressions before this would be accepted similar to the template-based widget rendering change. I don't see any references here or in this work. What is the status of those?- The documentation on Channels is considerably more than I would have accepted for a patch. What do you think is missing?- The channels branch is likely missing some tests around auth and session in particular, but a lot of the "main" stuff has tests; what would you need before accepting it?- asgiref quite literally has more tests than actual code we're using, so I don't see a problem there- Daphne is severely lacking in tests for WebSocket encoding/decoding support as writing a test harness for it is ridiculously hard and I was hoping to get some help on that now we finally have the MOSS money around.- There are no performance regression tests as by default the codepath in Django is the same as before (apart from runserver, which in my basic tests actually sped up); as long as you deploy using WSGI, you never even touch Channels code (which wasn't the original plan, but now is for sanity reasons).I would like some leeway on the external dependencies being fully tested considering that they are not beholden to Django's release cycle, but maybe because we're declaring them as core dependencies (I patched in install_requires into setup.py for them) we should be very strict? If this patch is going to be denied based on Daphne not having sufficient test coverage then I suspect we'll miss the deadline and have to foist this on the LTS instead, which I really don't want.Andrew
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/0b519047-96ee-4ac9-9230-0f487625a7e2%40googlegroups.com.
What about having asgiref and daphne as optional dependencies instead of
hard once and raising a proper exception "please install ..." when the
import fails?
```
try:
from asgiref import ...
except ImportError:
raise ImportError(
"Please ensure you installed asgiref to use this feature. Do so "
"with `pip install Django[channels]"
)
```
i.e. `pip install "Django[channels]"` ?
On Wed, May 4, 2016 at 8:26 AM, Mark Lavin <markd...@gmail.com> wrote:As noted in the PR there is at least one new setting, CHANNEL_SESSION_ENGINE, which is lacking documentation.That's been added now.The notes on deployment for "Running ASGI under WSGI" don't give any motivation why you would want to do this given that it doesn't support websockets in this case. Is there any advantages? Disadvantages?It lets you still have background tasks and non-WebSocket interface servers (or run websocket interfaces separately). I've expanded the docs to say this.
There are tests for the routing and request handling but there are no unittests that I can see for the auth/session handling,Yes, these are missing.Channel/Message/Group/ChannelLayerManager/custom JSON encoder classes and their usage,The ASGI conformance test suite takes care of this: https://github.com/andrewgodwin/django/blob/channels/tests/channels_tests/test_database_layer.py#L3 (that's also used for the inmemory module and asgi_redis in their test suites)
or the worker/runworker code.What would you suggest these tests look like? They can't spin up a worker and fire requests at it, and most of the worker code is interactive.
The changes to runserver aren't tested.Runserver in Django only has a single test, presumably for the same reason; I don't know how I can write a test for this considering the autoreload and extra process mechanisms we'd want to cover.
The new test utilities aren't tested either (yes that's a thing).I would say that the usage of those utilities in other tests means they are tested sufficiently, given that they're used sufficiently; writing a test for just the channel test case would look very similar to the basic tests elsewhere.
All new code should have tests. Isn't that Django's current standard?There's no clear standard, the docs just say the tests should "exercise all of the new features". I admit that auth/session needs tests - I'll get those done this week - but I don't see how we can reasonably test the interactive commands in an automated fashion given the current test harness (and lack of tests for things like runserver) in Django.
asgiref has one "basic" test around WSGI handling which the usage is documented in this change. I don't think anyone would dare say that covers potential edge cases in WSGI/HTTP handling. Not test related, the status code map is also missing many HTTP status codes.The WSGI handling part of asgiref is not considered complete, and I will probably remove the section about using it in the main Django docs. Only the inmemory backend should be considered used by Django.
Given that there is no guarantee of message delivery, I don't think this is suitable or recommended for using for background tasks. I don't think the Django docs should encourage that usage. At least without a strong warning that this isn't a good idea.There are tests for the routing and request handling but there are no unittests that I can see for the auth/session handling,Yes, these are missing.Channel/Message/Group/ChannelLayerManager/custom JSON encoder classes and their usage,The ASGI conformance test suite takes care of this: https://github.com/andrewgodwin/django/blob/channels/tests/channels_tests/test_database_layer.py#L3 (that's also used for the inmemory module and asgi_redis in their test suites)Yes the code is covered by integration/functional tests but again I feel like there should be unittests for this as well.
or the worker/runworker code.What would you suggest these tests look like? They can't spin up a worker and fire requests at it, and most of the worker code is interactive.It is certainly possible to fire up a worker in a subprocess and test it that way. Or you could create an instance of the Worker object and call its methods.
The changes to runserver aren't tested.Runserver in Django only has a single test, presumably for the same reason; I don't know how I can write a test for this considering the autoreload and extra process mechanisms we'd want to cover.The new test utilities aren't tested either (yes that's a thing).I would say that the usage of those utilities in other tests means they are tested sufficiently, given that they're used sufficiently; writing a test for just the channel test case would look very similar to the basic tests elsewhere.The existing test utilities (test client, asserting the number of queries, overriding settings, etc) have tests of their own https://github.com/django/django/blob/master/tests/test_utils/tests.py as do the extensions added by contrib.staticfiles. I don't see why Channels is special and therefore doesn't need them.
All new code should have tests. Isn't that Django's current standard?There's no clear standard, the docs just say the tests should "exercise all of the new features". I admit that auth/session needs tests - I'll get those done this week - but I don't see how we can reasonably test the interactive commands in an automated fashion given the current test harness (and lack of tests for things like runserver) in Django.To me it's a bad code smell when code is hard to test. I can't believe I'm having to convince anyone in this community about the value of testing.
asgiref has one "basic" test around WSGI handling which the usage is documented in this change. I don't think anyone would dare say that covers potential edge cases in WSGI/HTTP handling. Not test related, the status code map is also missing many HTTP status codes.The WSGI handling part of asgiref is not considered complete, and I will probably remove the section about using it in the main Django docs. Only the inmemory backend should be considered used by Django.This is exactly my point. This project is not complete. That is not at all clear from this PR or from looking at the asgiref README which only re-enforces my feeling that this dependency should not be a requirement when installing Django.
On Wed, May 4, 2016 at 11:52 AM, Mark Lavin <markd...@gmail.com> wrote:Given that there is no guarantee of message delivery, I don't think this is suitable or recommended for using for background tasks. I don't think the Django docs should encourage that usage. At least without a strong warning that this isn't a good idea.There are tests for the routing and request handling but there are no unittests that I can see for the auth/session handling,Yes, these are missing.Channel/Message/Group/ChannelLayerManager/custom JSON encoder classes and their usage,The ASGI conformance test suite takes care of this: https://github.com/andrewgodwin/django/blob/channels/tests/channels_tests/test_database_layer.py#L3 (that's also used for the inmemory module and asgi_redis in their test suites)Yes the code is covered by integration/functional tests but again I feel like there should be unittests for this as well.Those are unit tests as far as I'm concerned, and they're quite comprehensive - there's no smaller units of work to really write tests _for_ in any of these backends.or the worker/runworker code.What would you suggest these tests look like? They can't spin up a worker and fire requests at it, and most of the worker code is interactive.It is certainly possible to fire up a worker in a subprocess and test it that way. Or you could create an instance of the Worker object and call its methods.I'll look into this, but firing up subprocesses won't work as it suddenly introduces Redis as a test dependency (right now all tests use the in-memory backend).The changes to runserver aren't tested.Runserver in Django only has a single test, presumably for the same reason; I don't know how I can write a test for this considering the autoreload and extra process mechanisms we'd want to cover.The new test utilities aren't tested either (yes that's a thing).I would say that the usage of those utilities in other tests means they are tested sufficiently, given that they're used sufficiently; writing a test for just the channel test case would look very similar to the basic tests elsewhere.The existing test utilities (test client, asserting the number of queries, overriding settings, etc) have tests of their own https://github.com/django/django/blob/master/tests/test_utils/tests.py as do the extensions added by contrib.staticfiles. I don't see why Channels is special and therefore doesn't need them.I would counter that by saying that the Channels test handler code is only about 20 additional lines of logic and no new assertions, so the existing suite is more than enough.All new code should have tests. Isn't that Django's current standard?There's no clear standard, the docs just say the tests should "exercise all of the new features". I admit that auth/session needs tests - I'll get those done this week - but I don't see how we can reasonably test the interactive commands in an automated fashion given the current test harness (and lack of tests for things like runserver) in Django.To me it's a bad code smell when code is hard to test. I can't believe I'm having to convince anyone in this community about the value of testing.Well, it is hard to test, mostly due to the fact that testing websockets requires the test client to also be asynchronous as well as the server, and if they're in the same process that leads to trouble.I am very much aware of the value of testing, but I'm saying that in this case it's a large hurdle to climb, I believe the tradeoff is acceptable to merge it in for the alpha as it is and then get the MOSS program to pay some people to work on just this part so it gets the right level of attention.
appreciate
asgiref has one "basic" test around WSGI handling which the usage is documented in this change. I don't think anyone would dare say that covers potential edge cases in WSGI/HTTP handling. Not test related, the status code map is also missing many HTTP status codes.
The WSGI handling part of asgiref is not considered complete, and I will probably remove the section about using it in the main Django docs. Only the inmemory backend should be considered used by Django.
This is exactly my point. This project is not complete. That is not at all clear from this PR or from looking at the asgiref README which only re-enforces my feeling that this dependency should not be a requirement when installing Django.
I can (and do) appreciate the effort that's gone into this work while still feeling as though it isn't ready to be merged with Django. To be honest given that this PR contains almost no changes to Django itself and only adds new code, I don't understand why it can't live outside of Django while it continues to mature.
Major features merged into Django have generally never been as "perfect" as the standards required for smaller patches. There's a recognisation of the need for ongoing work, probably over the course of multiple versions, in order to perfect any major new feature. The effort involved in getting a patch like this to the point it can be merged at all is very significant. Many major patches (composite fields, templates widgets...) have previous not landed, sometimes multiple times, because they are nowhere near the standard that this channels patch is.
My feeling is that the core team agree with Andrew that this is an important direction for Django, and 1.10 is the right release to include it in. Bug fixes, additional tests and so on can all be added between alpha and final as needed. The important thing is that we have broad agreement that the feature is good, the design is right, and an understanding that shortcomings will be worked on over the next year or two.
Marc
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAFwN1uqpLNTLDuZzNQzH_4k_ZYzHx0pbuXzeKo7u80MjVMr3HA%40mail.gmail.com.
Major features merged into Django have generally never been as "perfect" as the standards required for smaller patches. There's a recognisation of the need for ongoing work, probably over the course of multiple versions, in order to perfect any major new feature. The effort involved in getting a patch like this to the point it can be merged at all is very significant. Many major patches (composite fields, templates widgets...) have previous not landed, sometimes multiple times, because they are nowhere near the standard that this channels patch is.
My feeling is that the core team agree with Andrew that this is an important direction for Django, and 1.10 is the right release to include it in. Bug fixes, additional tests and so on can all be added between alpha and final as needed. The important thing is that we have broad agreement that the feature is good, the design is right, and an understanding that shortcomings will be worked on over the next year or two.
May 16 | Django 1.10 alpha; feature freeze. |
June 17 | Django 1.10 beta; non-release blocking bug fix freeze. |
July 15 | Django 1.10 RC 1; translation string freeze. |
2+ weeks after RC1 |
Django 1.10 final (or RC 2, if needed). |
The future is hard to predict, but I'll add that I'm a tad nervous about this as well.
I'm completely inexperienced with channels at this time. Who else has a good grasp of the code right now and could help fix release blocking bugs if Andrew isn't available? Are we playing for any or all bug fixes? If these requests have to go through the MOSS committee, will that delay getting things fixed?
Migrations delayed the 1.7 release for months, and I'd like some vote of confidence that merging this at such a late stage won't cause similar delays.
Major features have never been perfect, no, but they have in the past typically gone through two paths to prove out their design/API/usefulness. One is as an established and mature third-party app such as messages, staticfiles, and django-secure. More recently the other has been through the DEP process: multiple templates (Jinja) and query expressions. Channels has done neither.Sorry if it seems that I've raised these issues late but I don't feel like there has been a good place for this discussion since the DEP process was circumvented. Most of the development for this has been in Andrew's space. I don't feel welcome to raise a dissenting opinion as a mere lowly member of the Django community.
It's pointless for me to continue to elaborate on the things I don't like about channels as I'm clearly in the minority and it's going to land. All I can do now is beg for these requirements to be optional.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/455347ad-689c-49c2-a1c0-ace3127a3583%40googlegroups.com.
I will admin that I haven’t been paying *close* attention to Andrew’s work - I’m aware of the broad strokes, and I’ve skimmed some of the design discussions, but I haven’t been keeping close tabs on things. From that (admittedly weak) position, the only counterargument that I’ve heard are performance concerns.
On Thursday, May 5, 2016, Russell Keith-Magee <rus...@keith-magee.com> wrote:I will admin that I haven’t been paying *close* attention to Andrew’s work - I’m aware of the broad strokes, and I’ve skimmed some of the design discussions, but I haven’t been keeping close tabs on things. From that (admittedly weak) position, the only counterargument that I’ve heard are performance concerns.I haven't brought this up before, but security is something we should discuss pre-merge.What I'm mainly worried about is malicious clients intentionally trying to choke the channels layer. I guess the approaches for DoS attack would fall under these categories:1. Try to generate large responses and read those response slowly.
2. Fire a large request, don't read the response.
3. Try to cause exceptions in various parts of the stack - if the worker never writes to the response channel, what will happen?
There are always DoS vectors, but checking there aren't easy ones should be done. The main concern with channels is potential resource leak.I found accidentally some presentations that seem relevant to thus discussion. I recently watched some presentations about high availability at Braintree. There are two presentations available, one from 2013 by Paul Gross, where he explains their approach to HA, and one from 2015 by Lionel Barrow, explaining what changed. Both are very interesting and highly recommended.The 2013 presentation introduces one key piece to HA at braintree, called Broxy. Broxy basically serves HTTP the same way as Daphne - write requests to tedis, and wait for response again through Redis.The 2015 representation explains what changed. They removed Broxy because it turned out to be conceptually complex and fragile. It might be their implementation. But there is certain level of both complexity and possible fragility about the design. On the other hand their story pretty much verifies that the design does scale.
All in all I'd feel a lot more confident if there were large production sites using the channels system, so that we shouldn't theorize on the excellence of the approach. Are there already production deployments out there?
Do you have a link to the presentation about them removing it?
On Thursday, May 5, 2016, Andrew Godwin <and...@aeracode.org> wrote:Do you have a link to the presentation about them removing it?https://youtu.be/839rskyJaro around 34 minutes and onwards, another version is https://youtu.be/2dG5HeM7MvA at 24 minutes.
They were tackling a bit different problem, so their lessons might not apply. Most importantly they weren't aiming for websockets at all, just for high availability and throughput for normal HTTP traffic. On the other hand, the architecture of broxy itself is pretty much the same as Daphne, and they felt they had problems with it.
The way we have approached recent feature additions is that we let them prove themselves outside of core. I think the crucial question is why Channels can't do this, that is why can't we wait some time and let actual production deployments prove the design?I know South is an example of why some features have a hard time living outside core. But DRF is an example that a feature crucial to modern web development can work very well outside of core.
I'm not convinced either way about whether putting this in core will help mature it and fix bugs more quickly or not. I don't have any sense of how the code might change after we merge it, but things get more complicated if we start selectively backporting somes fixes for Django's monthly bug fix releases. If channels continues to live externally, I assume there won't be the complication of a master vs. stable branch and that releases could happen more often. If this feature is exciting enough, whether or not its in Django itself shouldn't make too much of a difference, should it? Also, if we do include this in 1.10 and then start paying people using the Mozilla money, that might result in master changing even more aggressively compared to the stable branch at which point having a 1.10 release that's X months old in terms of the latest features might not be all that valuable.
Tell me if I'm wrong, but I get the sense that you are somewhat burned out maintaining this project on your own and you feel that merging this to Django will help offload your burden and attract other contributors. If that's the case, there's a possible solution in using some of the Mozilla money to try to get help in more of the code review/maintenance tasks. If we have an accepted DEP and plans to merge it to core in 1.11, I would try to get more involved with those projects too.
I don't see a problem in continuing to refine this externally even if that means it lands in an LTS. I sure would feel less overwhelmed if I didn't have to review the big patch up against the alpha deadline and/or if this threatens to delay the 1.10 release.
I'll start by saying I've enjoyed the contributions by Andrew over the years and believe he is a most excellent developer.
A couple months ago, around the same time that JKM started Andrew's repo (which is what got my attention) I decided to give Channels a try. I had a quick honeymoon with it because I had been working on a game and was using aiohttp. aiohttp provides support for websockets but doesn't manage relationships between them, which makes it hard to do anything without building a layer in between your app and the web. This layer ends up being somethings like Channels. Channels basically accomplished what I needed, which I was ecstatic about.
The honeymoon ended when I started using Channels to experiment on my local machine. The notion that I would now have to use a data store to run my app at all didn't feel right, even though I knew there would need to be data stored to maintain relationships between websockets, etc. I was disheartened when I learned Channels was potentially going to be merged into Django, because I finally got to that realization of the fact that there wasn't a "right way" to include baked-in web sockets behind a web server, only to find out that Channels was going to be called the "right way", forever (potentially).
So, let me ask a question: how many of the past 5 paid projects you worked on required websockets? For me, and for most of the people I know (not many), the answer is 0. I can think of a use case where it would have been nice (the "how many other users are viewing this page" feature, which we accomplished with slow Ajax polling), but I can think of nothing critical.
I ask that question because a good argument came up earlier, a comparison to NoSQL. NoSQL was in the same must-have craze, and then the tried and true standard in Django (Postgres) sort of answered that with huge gains in performance and scalability. Competition is great for this reason, but merging in competion is sort of like a business acquiring innovative. competitors as they emerge; it doesn't do much to further the company's true vision, because it's a defensive move.
I would recommend against merging Channels now, and potentially ever; not because it's not great code, but because A) it isn't necessary for the core, B) leaving it out of the core doesn't prevent it from being used, and C) it should be proven over time as an outside package (a different way of solving this could come by and proliferate while we have Channels in core, and then Django would be heading back down that hill towards the bloat (e.g., contrib) it worked so hard to remove over the years - not that Django was ever *truly* bloated).
What I would recommend is that we focus on fixing/abstracting/documenting things that make it hard to integrate something like Channels (e.g., things that Andrew has to monkey patch, etc.), because Channels *and* a hypothetical future websockets package could both take advantage of such an abstraction layer.
Political tirade follows:
Another thing I would never recommend is letting a grant destroy Django's ability to make a decision on its own. It absolutely disgusts me to think of Django becoming like Firefox, with "features" like Pocket and Hello that are driven by financial or political desires. Django should continue to look unemotionally at feature requests without regard for sunk cost fallacy (e.g., "<developer> already spent X time working on this...", etc.).
*JKM starred (not started) - stupid, stupid iPhone.