HH-196544 add dependency graph #658

712u3 · 2023-11-13T07:52:31Z

https://jira.hh.ru/browse/HH-196544

sintell · 2023-11-13T09:29:52Z

https://github.com/hhru/frontik/blob/master/docs/preprocessors.md

Вот тут очень нехватает изменений, чтобы отражало текущую ситуацию

bokshitsky · 2023-11-13T09:31:38Z

frontik/dependency_manager.py

+        self.finished = False
+        self.task: asyncio.Task | None = None
+
+    async def run(self, handler: PageHandler, topological_sorter: TopologicalSorter) -> None:


Есть вопрос ко всех схеме по поводу её самописности, но тут коммент к конкретному месту:
имхо хендлер не надо здесь отдельный параметром делать, думаю лучше, чтобы была какая-то зависимость, сразу зарезолвленная, которая вернет CurrentHandler

bokshitsky · 2023-11-13T09:32:54Z

frontik/dependency_manager.py

+
+        if asyncio.iscoroutinefunction(self.func):
+            if self.task is None:
+                self.task = asyncio.create_task(self.func(*self.args))


но ведь может быть и async function которая является синхронной зависимостью?
Например когда внутри есть какие-то походы, но их надо дождаться?

тут следующей строчкой сразу же await этой таски
или ты о чем?
таска нужна только из-за субграфа (а он только из "маст_би") чтобы если один граф уже стартанул зависимость, второй не запускал новую, а авейтил существующую

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

sintell · 2023-11-13T16:54:53Z

frontik/dependency_manager.py

+            topological_sorter.done(self)
+            return
+
+        for i in range(len(self.args)):


а есть причина, почему тут не for arg, i in enumerate(self.args)?

потому что модифицируем self.args в рамках цикла

ну так ты получаешь тот же самый индекс и можешь итерироваться, просто тебе не нужно писать лишнюю строчку arg = self.args[i]

я только перепутал, наоборот: for i, arg in enumerate(self.args):

sintell · 2023-11-13T16:57:42Z

frontik/dependency_manager.py

+        return make_full_name(self.func)
+
+
+def dep(dependency: Preprocessor | Callable | list[Callable]) -> Any:


А назови функцию, чтобы было понятно что она делает, не экономь на буквах

это название потом в сервисы в аргументы поедет, думаешь удобно будет если оно длинное?

т.е. условно в xhh

@preprocessor async def vacation_check(handler: PageHanler, session=make_dependency(get_session))

vs

@preprocessor async def vacation_check(handler: PageHanler, session=dep(get_session))

sintell · 2023-11-13T16:59:04Z

frontik/dependency_manager.py

+            arg = self.args[i]
+            if isinstance(arg, Dependency):
+                if not arg.finished:
+                    msg = f'Graph corrupted, run {self}, before finishing {arg}'


А зачем эта строчечка в отдельной переменной? Там за лимит длины строки чтоли вылазим? Может надо чот подтюнить?

это руфф поавтофиксил, https://docs.astral.sh/ruff/rules/f-string-in-exception/
типа дважды потом сообщение в логи
мне в целом не принципиально, можно вырубить

sintell · 2023-11-13T16:59:31Z

frontik/dependency_manager.py

+        return DependencyMarker(dependency)
+
+    msg = 'Bad dependency type, only func or list[func]'
+    raise ValueError(msg)


Тут точно не вылазим за длинну строки, зачем тут переменная?

sintell · 2023-11-13T17:03:28Z

frontik/dependency_manager.py

+            done, pending = await asyncio.wait(pending_tasks, return_when=asyncio.FIRST_COMPLETED)
+            for d in done:
+                if d.exception() is not None:
+                    raise d.exception()  # type: ignore


А почему тут тип заигнорен?

я не смог победить mypy)
он говорит Exception must be derived from BaseException [misc]
посмотрел в некоторых либах так делают

sintell · 2023-11-13T17:04:17Z

frontik/dependency_manager.py

+
+def get_dependency_graph(page_method_func: Callable, handler_cls: type) -> DependencyGraph:
+    """
+    build meta_graph or take ready deepcopy as main_graph


or take ready deepcopy это как?

имел в виду, что у нас есть что-то типа статического объекта
my_module.MyPageHandler.get_page
и если у него нету ._dep_graph, то мы новый строим, проверяем циклы
а если уже есть то делаем deepcopy этого ._dep_graph
чтобы модифицировать, отмечать кого выполнили, кого нет

sintell · 2023-11-13T17:06:15Z

frontik/dependency_manager.py

+    func_preprocessors = get_preprocessors(page_method_func)
+    handler_dependencies = getattr(handler_cls, 'dependencies', [])
+
+    analyze_dependency_params(meta_graph, root_dep, False, True)


А давай везде, где ты булевые аргументы передаешь — передадим их как kwargs?

sintell · 2023-11-13T17:08:34Z

frontik/dependency_manager.py

+    for dependency in chain(func_preprocessors, handler_dependencies):
+        register_sub_dependency(meta_graph, root_dep, dependency, False, True)
+
+    analyze_dependency_params(meta_graph, root_dep, True, False)


Давай переименуем метод, чтобы было понятно, что он не только что-то анализирует, но и меняет что-то внутри аргументов

sintell · 2023-11-13T17:10:18Z

frontik/dependency_manager.py

+    handler_dependencies = getattr(handler_cls, 'dependencies', [])
+
+    analyze_dependency_params(meta_graph, root_dep, False, True)
+    for dependency in chain(func_preprocessors, handler_dependencies):


Я правильно понимаю, что в одном цикле нельзя как раз из-за того, что нужно analize_dependency_params вызвать, которая stateful, и состояние может поменяться между вызовами?

думаю этот вопрос связан со следующим про tier1, подумаю как там получше коммент написать

в целом зависимости в некотором смысле синглтоны, мы по имени функции можем получить объект типа Dependency. Т.е. если в графе много раз встречается какой-нибудь get_session это будет единая зависимость, которая выполнится один раз
Но бывает такое что у нас не нормальная зависимость, а "полученная из фабрики" типа такого

def preproc_factory(params): @preprocessor def real_preproc(handler): pass return real_preproc

Такая фабрика выдает разные функции с одним именем. В графе может несколько раз в качестве препроцессора/зависимости встретиться вызов фабрики. Выполнять в итоге несколько раз?
В старой схеме выполняли 1 раз если одно имя.
Поэтому сделал так что если уже регистрировали в графе что-то с конкретным именем, то следующие игнорируем. Чтобы мочь управлять какая конкретно зарегистрируется (где-то было нужно), сделал так, что то что явно указано в сигнатуре (а не в глубинах графа) имеет больший приоритет

Чтобы собрать сначала приоритетные фабрики, нужно пройтись по верхам не заходя вглубь. А полный рекурентный обход сначала в ветви идет, так что два прогона, плоский и глубокий

sintell · 2023-11-13T17:12:40Z

frontik/dependency_manager.py

+    add link to graph
+    deep scan sub_dep parameters
+    if there are two different dependency with same name (factory's dep), then leave only first
+    if they both from tier1 signature raise ValueError('Dependency conflict')


Что такое tier1 signature ?

712u3 · 2023-11-15T09:50:01Z

Была просьба добавить этот текст

Q

не сложно ли будет поддерживать алгоритм
добавляет проблем к статической передаче параметров внутрь (аналог фастапи)

A

Уже чуток упростил в ревьюфиксах. Можно будет упростить еще, если а) избавимся от старыз препроцов б) отпадет необходимость в рантайме собирать суб_граф и мерджить с основным
Уже поправил в ревьюфиксах, в графе есть special_dependencies, можно туда добавлять всякие query_reader/cookie_reader/etc

hh-sonar · 2023-11-16T16:21:02Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

712u3 requested a review from a team as a code owner November 13, 2023 07:52

bokshitsky reviewed Nov 13, 2023

View reviewed changes

sintell reviewed Nov 13, 2023

View reviewed changes

712u3 force-pushed the HH-196544 branch 2 times, most recently from 54f34ea to 2cdc0c4 Compare November 15, 2023 15:04

HH-196544 add dependency graph

36c728a

712u3 force-pushed the HH-196544 branch from 484c1d4 to 36c728a Compare November 16, 2023 16:20

nicholasgribanov approved these changes Nov 16, 2023

View reviewed changes

hhrelease merged commit 7fd2b9b into master Nov 17, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HH-196544 add dependency graph #658

HH-196544 add dependency graph #658

712u3 commented Nov 13, 2023

sintell commented Nov 13, 2023

bokshitsky Nov 13, 2023

bokshitsky Nov 13, 2023

712u3 Nov 13, 2023

712u3 Nov 13, 2023

sintell Nov 13, 2023 •

edited

Loading

712u3 Nov 13, 2023

sintell Nov 15, 2023

sintell Nov 15, 2023 •

edited

Loading

sintell Nov 15, 2023

sintell Nov 13, 2023

712u3 Nov 13, 2023

sintell Nov 13, 2023

712u3 Nov 13, 2023

sintell Nov 13, 2023

sintell Nov 13, 2023

712u3 Nov 13, 2023

sintell Nov 13, 2023

712u3 Nov 13, 2023

sintell Nov 13, 2023

sintell Nov 13, 2023

sintell Nov 13, 2023

712u3 Nov 13, 2023

sintell Nov 13, 2023

712u3 commented Nov 15, 2023

hh-sonar bot commented Nov 16, 2023

		return make_full_name(self.func)


		def dep(dependency: Preprocessor \| Callable \| list[Callable]) -> Any:

HH-196544 add dependency graph #658

HH-196544 add dependency graph #658

Conversation

712u3 commented Nov 13, 2023

sintell commented Nov 13, 2023

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

sintell Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sintell Nov 15, 2023 • edited Loading

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

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

712u3 commented Nov 15, 2023

hh-sonar bot commented Nov 16, 2023

sintell Nov 13, 2023 •

edited

Loading

sintell Nov 15, 2023 •

edited

Loading