Support for unittest -k option

2019-03-11 Thread François Freitag
Hi Django Devs,

https://code.djangoproject.com/ticket/30245 suggests supporting Python
unittest `-k` option, to selectively run tests matching a keyword.

Currently, `-k` is the shorthand for `--keepdb` in Django.
A `--filter` flag was suggested to preserve backward compatibility.
Carlton suggested removing the `-k` option from `--keepdb` and reusing
it for unittest `-k`. That would follow unittest more closely, reduce
user confusion and ease maintenance.

What do you think is best? Do you see other options?

If re-taking the `-k` option for unittest `-k`, should a new shorthand
be introduced for `--keepdb`?

Thanks for your time,
François

-- 
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 https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/f17da2c4-b311-75ca-8a52-33504391eb0c%40gmail.com.
For more options, visit https://groups.google.com/d/optout.


Should ModelChoiceIterator cache its QuerySet?

2018-04-02 Thread François Freitag
Hi,

I'm having mixed feelings toward ModelChoiceIterator's implementation.
That custom iterator intends to fetch choices defined by the queryset
argument of a ChoiceField.

Issue
=

Currently, the expectation seems to be that the latest data is queried
from the database each time the choices attribute of a ChoiceField
instance is evaluated. That's best described by
ModelFormBasicTests.test_runtime_choicefield_populated [1]. Besides,
#28312 reports reusing [...] the cached QuerySet in ModelChoiceIterator
as a bug [2].

My expectation is that once evaluated, the QuerySet stays cached for the
ModelChoiceIterator instance's lifetime (i.e. in
test_runtime_choicefield_populated the w_bernstein entry would not be
visible in the second f.as_ul() output). Unaware of #28312, I created
#29159 to avoid making a duplicate query when calling list() on a
ModelChoiceIterator's choices. The fix for #29159 [3] makes a step
toward caching the QuerySet results on the ModelChoiceIterator instance,
which goes against the existing expectation.

Questions
=

Before to break the existing assumption further, I would like to gather
opinions on the following two options.

1. Should a new patch be submitted for #29159 [4] to use `.count()` for
`__len__`, thus reverting back to the original behaviour?

2. Should ModelChoiceIterator be updated to cache its QuerySet? (reverts
results of #23623 [5])
2a. Removing test_runtime_choicefield_populated
2b. Marking #28312 as fixed (because the len() and list() content would
be consistent)

My opinion (option 2)
=

AFAICT, reusing the ModelChoiceIterator queryset results, instead of
issuing a new query whenever the ModelChoiceIterator instance is
evaluated, is the only way to guarantee results consistency across calls
to the choices attribute of a ChoiceField instance.
If the QuerySet results were to change, there could be mismatches, for
example between the choices count and the actual number of choices
offered to the user. That would lead to subtle bugs under concurrent
activity, where a new entry is inserted between the counting and the
rendering.

However, this option means breaking the current assumption that choices
will be as "fresh" as possible. For instance, a choice created after the
ModelChoiceIterator instance was evaluated the first time would not be
available for selection, making the following pattern incorrect (untested):

```
f = MyChoiceField(Category.objects.all())
if 'form' not in {val for val, label in f.choices}:  # Evaluate choices
Category.objects.create('Django')
f.as_ul()  # Won't display Django, because the first choices evaluation
   # created a cached queryset.
```

IMHO, the freshness isn't so much of an issue for most use cases,
because the ModelChoiceIterator will likely be consumed during the
template rendering, so the choices should be just as fresh.

This option also means consuming more memory, because Python objects
would stay cached on the QuerySet (we currently use .iterator() to load
the choices when possible). It don't think that's too bad, since
ModelChoiceIterator instances evaluation is lazy, thus the lifespan of
the choices cache should be short.

I don't expect that loading even thousands of choices would lead to a
major memory overhead. SQLite and MariaDB/MySQL adapters will store
database results in memory anyway, so the overhead would be on storing
Python instances versus storing text for these backends.
If memory really is a concern, I think significant savings can be
achieved using `.defer()` to reduce the size of the Python objects or
filtering/limiting the choices QuerySet. Another option is to use a
custom ModelChoiceIterator subclass to fetch the results using
`.iterator()`.

I would prefer Django to have consistent results by default, even though
that means more memory usage. What do you think?

Kind regards,
François Freitag

[1]
https://github.com/django/django/blob/73cb62a33197652a3c8261dbf052d7eb75e26139/tests/model_forms/tests.py#L1483-L1538
[2] https://code.djangoproject.com/ticket/28312
[3] https://code.djangoproject.com/ticket/29159
[4]
https://github.com/django/django/commit/a2e97abd8149e78071806a52282a24c27fe8236b
[5] https://code.djangoproject.com/ticket/23623

-- 
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 https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/88c86674-876f-a394-cb47-b418897b023f%40gmail.com.
For more options, visit https://groups.google.com/d/optout.


Re: Refactoring the autoreloader

2017-10-02 Thread François Freitag

Hi Tom,

Thank you for the great summary.

> 2. Add support for the "watchdog" library as a replacement for
> pyinotify. Watchdog implements file system notifications for all major
> platforms using a fairly simple API, so we can remove polling and have
> instant reloading. Also support Watchman, a notification Daemon from
> Facebook.

Filesystem polling is required for some setup, such as mounting code 
using NFS or rsync, for example using vagrant synced folders[1]. 
Although it does not prevent from using the watchdog library, which 
provides a PollingObserver[2], I think it's worth keeping that use case 
in mind. The PR has a StatReloader which seem able to handle filesystem 
polling, I would suggest either keeping it or delegating the polling to 
watchdog.


[1] https://www.vagrantup.com/docs/synced-folders/nfs.html
[2] 
https://pythonhosted.org/watchdog/api.html#module-watchdog.observers.polling


Cheers,
François

On 09/29/2017 12:03 PM, Tom Forbes wrote:

Hello,
I've been thinking on and off about how to improve the autoreloader 
implementation and I wanted to gather some feedback on potential solutions.


For some background, Django uses a fairly basic autoreload 
implementation that simply polls the last modified time for loaded 
Python files once a second. While this isn't the most efficient, it does 
work and has worked quite well for a long time. When running manage.py 
runserver, the autoreloader will launch a child "manage.py" with the 
same arguments and this child process actually runs Django and serves 
requests. To reload, the child process exits with exit code 3 and the 
parent restarts it. The code is some of the oldest in Django, with a 
fair bit of it not touched in 9-12 years.


While it works (and I'm a believer in "if it isn't broke don't fix it") 
there are some architectural and performance issues with the current code:


- Polling every second is not very efficient
- Detecting when the child process has exited during startup (i.e 
problem in settings.py) is problematic and the code is rather nasty
- i18n files are 'reloaded' when they change in a rather hacky way 
(resetting private attributes in another module)
- There is limited support for extending the current implementation, and 
there are cases during development where the parent autoreloader will 
terminate.


I don't want this email to be too long, so I'm going to summarize what I 
think would be a good approach to tackling these problems.


1. Refactor the current implementation by removing `pyinotify`, 
redundant python 2 checks and implement a 'file_changed' signal so other 
parts of Django can react to file changes (i.e the i18n module can 
handle resetting it's own state).
2. Add support for the "watchdog" library as a replacement for 
pyinotify. Watchdog implements file system notifications for all major 
platforms using a fairly simple API, so we can remove polling and have 
instant reloading. Also support Watchman, a notification Daemon from 
Facebook.
3. Add support for more advanced features, like proper handing of 
startup errors and socket sharing.


I've got a merge request that implements all three stages as a proof of 
concept, but I think it's far too much a change to be done at once and 
should be done carefully stage by stage. One and two are fairly simple 
to implement, but three requires see careful consideration as to the 
best approach (this message is long enough already, I don't want to 
describe them here).


Does anyone have any feedback on these ideas? Is it worth persuing even 
if the current implementation works ok-ish?


--
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 https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAFNZOJMT9qDk-4pKKXSJysEQCmd6CGxMZBYZs_7BQs_WbAqL6g%40mail.gmail.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 https://groups.google.com/group/django-developers.
To view this discussion on the web visit