Re: request for API review of streaming responses additions

[Aymeric Augustin]

Hello,

I have three main comments on this patch.

1) About the general API design

This patch adds 7 new “stream” public APIs that duplicate the current “render”
APIs. Adding a stream=False argument to the current APIs would be a more
lightweight alternative. Passing stream=True would enable the streaming
behavior. Was this alternative considered and if so, why was it rejected?

I think it would make the patch less intrusive and keep the documentation more
readable. Specifically I'm concerned about making several parts of the
documentation twice longer for a relatively uncommon need.

It's unclear to me why TemplateView gets a StreamingTemplateView sibling while
other generic class base views that inherit TemplateResponseMixin don't. All
GCBVs except View and RedirectView could get a Streaming version. Even if the
initial patch doesn't do it, someone will do it eventually, which amounts to
13 additional classes.

I'm -1 on the concept of duplicating every Django API that deals with HTTP
responses. That design doesn't strike a good balance between simplicity for
newcomers and power for advanced users.

2) About third-party template engines

How should third-party template engines that don't support streaming rendering
behave? Neither the code nor the release notes provide any guidance on this
issue. I suggest to add a stream() method that raises NotImplementedError to
the base class for template backends.

The current patch implements a stream() method that doesn't actually stream in
the DummyBackend and a boilerplate render() method that concatenates a list of
one element. This is bad, all the more since the dummy backend is intended to
be a template for third-party engines.

All template backends have the same copy-pasted render() method. Perhaps it's
a symptom of a problem with the API. Perhaps it should just be pulled to the
base class.

3) About performance

I think benchmarks are required:

a. to ensure that this change doesn't degrade performance for the traditional
  rendering mode

Yann Malet's recent bug report shows that even modest performance regressions
in template rendering can be a real problem for users.

b. to assess the performance of streaming rendering which could be
  unexpectedly slow on a realistic network (try over 3G tethering)

I'm bringing this up because streaming rendering will often yield many small
chunks. If each of these ends up in its own TCP packet -- which is going to
happen unless the application server provides additional buffering -- I expect
poor performance. If that's confirmed, it should be mentioned in the docs.
Streaming rendering will still be useful for rendering gigantic amounts of
data. But it isn't the performance optimization it may look like.

-- 
Aymeric.



> On 19 août 2015, at 00:18, Tim Graham  wrote:
> 
> I'd like to ask for a high-level API review of some proposed streaming API
> additions (I have already given the patch a couple of detailed reviews, but 
> other eyes would be welcome on the details as well).
> 
> Summary:
> 
> * django.views.generic.base.StreamingTemplateView to stream a template
>   rather than render it.
> 
> * A Template.stream() method, a django.template.loader.stream() function,
>   and django.shortcuts.stream_to_response() and
>   django.shortcuts.stream() shortcuts.
> 
> * django.template.response.StreamingTemplateResponse and
>   django.template.response.SimpleStreamingTemplateResponse classes to
>   handle streaming of templates.
> 
> Pull request:
> https://github.com/django/django/pull/4783 
> 
> 
> Thanks!
> 
> -- 
> 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-developers+unsubscr...@googlegroups.com 
> .
> To post to this group, send email to django-developers@googlegroups.com 
> .
> Visit this group at http://groups.google.com/group/django-developers 
> .
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/django-developers/772cae79-1941-4a6a-94a8-b82d5e767aa1%40googlegroups.com
>  
> .
> For more options, visit https://groups.google.com/d/optout 
> .

-- 
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/

Re: The hypothetical models.MultiField: is there a rationale to avoid it ? Or could it make it into future releases ?

[schinckel]

As it turns out, this is the approach I've settled on for my Postgres 
Composite Type/Field implementation.

https://bitbucket.org/schinckel/django-postgres/src/6a6078d8a2cc30bfb61d093354b8228f76484a0c/postgres/fields/composite_field.py?at=default

Some issues I have identified are:

* it's not really possible at this stage to have something that triggers a 
migration operation to create the type. In practice I've just been writing 
that migration as a RunSQL operation, but it would be nice to have this 
included in the project state in some way. 

* There is a fair bit of duplicated code between the composite field 
metaclass and the django model metaclass. It's possible there are better 
ways to handle this.

I haven't done anything with querying/lookups at this stage, but that's an 
exciting possible feature.

I also blogged about this last year:

http://schinckel.net/2014/09/24/using-postgres-composite-types-in-django/


Matt.


On Friday, August 21, 2015 at 8:41:23 PM UTC+9:30, Anssi Kääriäinen wrote:
>
> I've been thinking how we could move forward with composite fields without 
> requiring too much changes in one go (large changes tend to never happen).
>
> It seems the first step forward would be to introduce a composite field 
> with the following properties:
>
> class Foo(models.Model):
> x = models.IntegerField()
> y = models.IntegerField()
> point = models.CompositeField((x, y))
>
> foo.point would always return a named tuplet. You could customize the 
> return value to a full fledged object by implementing a custom subclass of 
> CompositeField, but Django will not have any opinion on what should happen 
> when you do:
> foo.x = 1
> foo.y = 2
> foo.point.x = 2
> foo.point.x == foo.x ???
>
> this was one of the main concerns in the DEP.
>
> In addition, on query side you should be able to use 
> .filter(point__x__lte=...), and you should be able to select 
> .values('point'). We also need migrations support, and likely changes in a 
> lot of other places in Django.
>
> The draft DEPs for this feature shouldn't be used as definite resource 
> when implementing this feature.
>
>  - Anssi
>
> On Thursday, August 20, 2015 at 6:49:11 PM UTC+3, Aron Podrigal wrote:
>>
>> Have a look at [1] it is a composite field implementation.
>>
>> [1] 
>> https://groups.google.com/forum/m/#!msg/django-developers/MZUcOE6-7GY/sZkBaHvC8wgJ
>> [2] 
>> https://github.com/django/deps/blob/master/draft/0191-composite-fields.rst
>> [3] 
>> https://github.com/django/deps/blob/master/draft/0192-standalone-composite-fields.rst
>> On Aug 20, 2015 10:31 AM,  wrote:
>>
>>>
>>>
>>> Le mardi 18 août 2015 01:36:28 UTC+2, Tim Graham a écrit :

 I think the general idea is captured in ticket #5929 -- Allow Fields to 
 use multiple db columns (complex datatypes). Is that the gist of your 
 proposal?

>>>
>>> Thank you for this link! It seems to discuss the same end result as what 
>>> I tried to present in my first message: the ability to have a single 
>>> models.Field managing an arbitrary number of DB columns under the hood.
>>>
>>> The proposed approach is perhaps a bit different: if I understood the 
>>> ticked correctly, it proposes to change the base Field class to make it 
>>> possible, when deriving from it, to manage one or several DB columns. My 
>>> first idea was more to mimic the composite pattern implementation already 
>>> in use with forms.MultiValueField:
>>> * The models.Field *leaf* classes would still manage a single DB column.
>>> * Introduce a models.MultiField class, which is a container of 
>>> models.Field classes (be it leaf classes or other MultiField classes). This 
>>> container would address the multiple columns indirectly, through the 
>>> interface of the composing fields. And, to the eyes of the rest of the 
>>> code, it would behave as a normal field, notably offering the to_python() 
>>> feature, hiding the composition in its implementation details.
>>>
>>> I did not take time yet to try and assemble a prototype of this idea; In 
>>> fact, I first wanted to confirm if such approach has not already been 
>>> rejected in the past, before investing work in it ;) 
>>>
>>> Does it sound like a feasible/interesting idea ? Or is there a good 
>>> reason not to do it / too many obvious technical complications that I did 
>>> not foresee ?
>>>
>>> Thank you for reading,
>>>   Ad
>>>
>>>
 https://code.djangoproject.com/ticket/5929

 On Monday, August 17, 2015 at 5:11:01 AM UTC-4, boito...@gmail.com 
 wrote:
>
> Hi,
>
>   While implementing  our collection management system based on 
> Django, we are always excited by the extensibility of the framework.
>   Most recently, we were exposed to the *forms.MultiValueField* and* 
> widgets.MultiWidget*, that seem to offer composition capacities to 
> users of the *form* and *widget* layers. Yet, we did not find any 
> equivalent in the *model* layer, which seemed a