Fixed/reworked VkTools.get_all_slow_iter method by hdk5 · Pull Request #121 · python273/vk_api

hdk5 · 2018-03-19T04:11:24Z

Сначала я заметил, что в текущей реализации обрезаются первые max_count результатов. Это исправил в первом коммите.

Затем, мне там же не понравились две вещи:

Отдельный запрос только для того, чтобы узнать количество результатов
Во время выполнения цикла количество элементов может измениться, и это надо учитывать

Так что вторым коммитом идёт моя реализация этого метода

hdk5 · 2018-03-19T04:18:04Z

Накосячил с мержем в свой форк, прошу прощения. К ПРу относятся три последних

python273 · 2018-03-20T04:08:29Z

Может еще посмотришь execute версию? Думаю там тоже можно count_diff сделать

hdk5 · 2018-03-20T05:48:22Z

Посмотрю

hdk5 · 2018-03-20T08:36:58Z

Теперь код внутри передаваемой на сервер функции аналогичен тому, что внутри get_all_slow_iter с ограничением на 25 вызовов.
А методы get_all_iter и get_all_iter_slow отличаются двумя строчками, одна из которых в докстринге. Это исправь сам тот вид, который сочтёшь нужным

python273 · 2018-03-20T09:10:11Z

+    var params = %(values)s,
+        calls = 0,
+        items = [],
+        count, new_count, count_diff, response;


Может чтобы не копировать логику в python функцию, в аргументы добавить count? Тогда на первом шаге вк функция будет вызываться с count == None, а дальше с count из ответа

+ offset. Еще можно возвращать need_more с true, если нужно еще раз вызывать вк функцию. Тогда будет:

values['offset'] = 0 count = None need_more = True while need_more: response = ...(..., count=count) need_more = response['need_more'] values['offset'] = response['offset'] count = response['count'] ...

hdk5 · 2018-03-20T10:20:45Z

Не совсем понимаю, зачем need_more, если всё равно сравнение count и offset должно идти правильно

hdk5 · 2018-03-20T10:25:42Z

А вообще, я тут осознала, что если удалить некоторые старые результаты и добавить новые так, что их количество не изменится, то правильность обхода будет всё равно нарушена. Тут уж, если по хорошему, надо смотреть на id у результатов, когда они есть

hdk5 · 2018-03-20T10:37:05Z

И ещё, если метод вызвался с одними параметрами в один момент, не значит что в следующую секунду я не попаду в чс/альбом будет удалён/etc.

Ух, сколько проблем то.

python273 · 2018-03-20T11:15:12Z

-        count_diff = new_count - count;
+        var response = API.%(method)s(params), 
+            new_count = response.count,
+            count_diff = (count == null : 0 ? new_count - count);


разве не something ? iftrue : iffalse?

Да, точно

python273 · 2018-03-20T11:18:24Z

Не совсем понимаю, зачем need_more, если всё равно сравнение count и offset должно идти правильно

Ну я имел в виду убрать логику count_diff из python функции, чтобы оно не дублировалось

если удалить некоторые старые результаты и добавить новые так, что их количество не изменится

Хм, мне кажется такое не особо вероятно. Максимум первые X итемов будут не актуальные, но это уже скорее всего будет скравлено в следующие разы (если вообще нужно получать обновления)

И ещё, если метод вызвался с одними параметрами в один момент, не значит что в следующую секунду я не попаду в чс/альбом будет удалён/etc.

Ну тоже не сильно вероятно. В любом случае нужно ловить ошибки при кравлинге

hdk5 · 2018-03-20T11:30:16Z

Ну я имел в виду убрать логику count_diff из python функции, чтобы оно не дублировалось

Оно должно быть в обоих местах. Изменение набора может произойти как в vk-функции (что тоже маловероятно), так и в питоновской функции, пока что-то происходит между yield.

python273 · 2018-03-20T11:42:02Z

так и в питоновской функции, пока что-то происходит между yield.

Но если возвращать последний offset из вк функции и перезаписывать в values, то в python функции count_diff не нужен, не?

hdk5 · 2018-03-20T11:53:17Z

Да, точно, теперь понимаю

python273 · 2018-03-20T17:41:03Z

Там еще предложение есть: #122
start_message_id ломает проверку offset + max_count > count. И с отрицательным offset проверка не будет работать. Наверное более универсально будет проверять len(items) < max_count, но так лишний запрос будет, если общее количество кратно max_count. Либо сразу 2 проверки делать 🤔

hdk5 · 2018-03-21T04:22:04Z

А не получится ли доработать функции так, чтобы была возможность указать, что требуется получать итемы с отрицательным оффсетом: добавить аргумент, за это отвечающий, и исправить сами функции?

hdk5 · 2018-03-21T05:59:11Z

То есть, что-то вроде этого: с оффсетом работать как и раньше, к апи обращаться с отрицательным оффсетом, если нужно, заканчивать всё, если ответ пустой

python273 · 2018-03-22T11:18:22Z

+            count_diff = new_count - (count or new_count)
+
+            if new_count == 0:
+                break


А для чего эта проверка?

python273 · 2018-03-22T11:42:37Z

                yield item

-            if limit and items_count >= limit:
+            if len(response_items) < max_count:


if len(items) < max_count:
Вроде при прошлой проверке если бы добавился новый итем, то остановилось бы из-за items = response[key][count_diff:]

python273 · 2018-03-22T11:44:05Z

Отрефакторил, чтобы было проще понять что происходит. Можешь посмотреть все ли ок и я мержну

python273 · 2018-03-22T11:56:05Z

negative_offset вместе с messages.getHistory + start_message_id не совсем верно работает. В первом запросе загрузит (max_count - 1) сообщений после start_message_id и только потом будут сообщения до start_message_id.

Но не уверен нужно ли фиксить. Можно начинать с -max_count смещения, но тогда сообщение с айди start_message_id не будет в items. Хотя вроде и не должно быть 🤔

Интересно есть ли у других методов отрицательный offset

hdk5 · 2018-03-22T12:59:10Z

Но не уверен нужно ли фиксить. Можно начинать с -max_count смещения, но тогда сообщение с айди start_message_id не будет в items. Хотя вроде и не должно быть 🤔

Да, всё так на самом деле

python273 · 2018-03-22T14:12:07Z

                )

-            items = response['items']
+            items = response[key]


Функция всегда 'items' возвращает

Ну вроде бы и да, но изначально этот метод принимал ключ как параметр. Сам я сходу не нахожу методов апи, возвращающих массив под каким-то иным именем.

Где-то было users, но я не про это. Тут вызывается вк функция, она всегда items возвращает. В вк функции используется key

А, точно, не обратил внимания

python273 · 2018-03-22T14:12:47Z

                yield item

-            if len(response_items) < max_count:
+            if len(response_items) < max_count - count_diff:


#121 (comment) ?

Разве это не должно исправить преждевременный выход?

Ну там уже проверка response_items вместо items, так что, как я понимаю, не нужно вычитать count_diff

Да, точно, не обратил внимания

python273 · 2018-03-23T04:17:11Z

^ там выше 2 коммента к коду

hdk5 · 2018-03-23T07:15:28Z

+            ri = response.%(key)s.slice(count_diff);
+            items = items + ri;
+            offset = offset + params.count + count_diff;
+            if (ri.length < params.count - count_diff) {


Но тогда здесь вычитать count_diff всё же нужно

Точно, там хотел немного по другому сделать

python273 · 2018-03-23T11:51:41Z

@hdk5 можешь посмотреть все ли ок и мержну

hdk5 · 2018-03-23T12:38:41Z

Вроде всё работает как ожидается 👍

xkord · 2018-03-26T20:05:12Z

Какое ожидаемое поведения для negative_offset = True, при условии, что выставлен start_message_id > 0 ?
В моем понимании, если значение start_message_id > 0, то, например, для messages.getHistory, tools.get_all должен вернуть историю начиная со start_message_id и до последнего сообщения в переписке. В текущей реализации это не работает. Возвращается история c самого начала и до start_message_id включительно.

r = tools.get_all('messages.getHistory', 200, {'user_id': user_id, 'start_message_id': 5}, negative_offset=True) 
print(json.dumps(r, ensure_ascii=False, indent=4))

В данном примере возвращаются сообщения с идентификаторами с 1 по 5.
Спасибо.

hdk5 · 2018-03-26T20:20:01Z

Какое ожидаемое поведения для negative_offset = True, при условии, что выставлен start_message_id?

Получение сообщений от start_message_id (не включительно) до наиболее нового (включительно)

xkord · 2018-03-26T20:21:34Z

Значит, вероятно, в текущем алгоритме есть ошибка.

hdk5 · 2018-03-26T20:37:18Z

Да, действительно. get_all_slow работает правильно, сейчас посмотрю что не так с execute

hdk5 · 2018-03-26T20:40:44Z

+        values['count'] = max_count

-        items_count = 0
        offset = 0


offset = max_count if negative_offset else 0

Сделаешь PR?

sakost · 2019-07-02T20:41:53Z

Алгоритм с exeute по-прежнему работает неверно :)

python273 reviewed Mar 20, 2018

View reviewed changes

hdk5 added 9 commits March 22, 2018 10:42

Fix for VkTools.get_all_slow method (stripping first elements)

37a4e6c

Reworked VkTools.get_all_slow_iter method

101777b

get_all_slow: result count could also decrease

02116fb

Get all offset fix for execute version

4d21d5c

Make old count count

060da63

js's ternary op whoops

ca539fc

Move out count diff logic from VkTools.get_all_iter to remote function

1ad3090

Negative offset for get_all_slow

54c6312

Negative offset and break condition

1fc3fb5

python273 reviewed Mar 22, 2018

View reviewed changes

Refactor&Fix VkTools methods

62430f4

python273 reviewed Mar 22, 2018

View reviewed changes

hdk5 added 2 commits March 22, 2018 15:21

Break condition fix

aaedb78

Changed initial negative offset

aa1d7a3

python273 reviewed Mar 22, 2018

View reviewed changes

Fixed get_all_iter response key

a0ee22d

hdk5 commented Mar 23, 2018

View reviewed changes

Refactor VkTools methods

aed5b6e

python273 merged commit c3b4f6f into python273:master Mar 23, 2018

hdk5 commented Mar 26, 2018

View reviewed changes

Conversation

hdk5 commented Mar 19, 2018

Uh oh!

hdk5 commented Mar 19, 2018

Uh oh!

python273 commented Mar 20, 2018

Uh oh!

hdk5 commented Mar 20, 2018

Uh oh!

hdk5 commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

python273 Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdk5 commented Mar 20, 2018

Uh oh!

hdk5 commented Mar 20, 2018

Uh oh!

hdk5 commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

python273 commented Mar 20, 2018

Uh oh!

hdk5 commented Mar 20, 2018

Uh oh!

python273 commented Mar 20, 2018

Uh oh!

hdk5 commented Mar 20, 2018

Uh oh!

python273 commented Mar 20, 2018

Uh oh!

hdk5 commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdk5 commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

python273 commented Mar 22, 2018

Uh oh!

python273 commented Mar 22, 2018

Uh oh!

hdk5 commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

python273 commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

python273 commented Mar 23, 2018

Uh oh!

hdk5 commented Mar 23, 2018

Uh oh!

xkord commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

python273 Mar 20, 2018 •

edited

Loading

hdk5 commented Mar 21, 2018 •

edited

Loading

xkord commented Mar 26, 2018 •

edited

Loading