From bf5e4170946e0b9720b11e9a24025232a7258ad6 Mon Sep 17 00:00:00 2001 From: pikaliov Date: Wed, 29 Apr 2026 15:15:40 +0300 Subject: [PATCH] Add new pre_refactor analysis.md --- pre_refactor_analysis.md | 297 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 297 insertions(+) create mode 100644 pre_refactor_analysis.md diff --git a/pre_refactor_analysis.md b/pre_refactor_analysis.md new file mode 100644 index 0000000..1ea8615 --- /dev/null +++ b/pre_refactor_analysis.md @@ -0,0 +1,297 @@ +# Шаг 1 — Что не соответствует стандарту в текущем состоянии + +> **Только диагностика.** Никаких решений по рефакторингу — это пойдёт на следующих шагах. Здесь только: что есть сейчас, какое правило стандарта нарушено, и каков масштаб проблемы. + +--- + +## Текущее состояние + +| Что | Где | Сколько | +|---|---|---| +| Гин-конфиги (взаимовключаемые через `include`) | `conf/` | 12 файлов: `balanced.gin`, `baseline_no_text.gin`, `text_heavy.gin` (v2 UAV-GeoLoc), `gtauav_balanced.gin`, `gtauav_baseline.gin`, `gtauav_balanced_asym.gin`, `gtauav_baseline_asym.gin`, `gtauav_balanced_stripnet.gin`, `gtauav_balanced_stripnet_unfrozen.gin`, `gtauav_baseline_stripnet.gin`, `gtauav_baseline_stripnet_unfrozen.gin`, `gtauav_text_heavy.gin`, `gtauav_image_heavy.gin` (по скриншоту в локальной копии есть ещё 4 sofia-варианта) | +| Главный `@gin.configurable + @dataclass` | `src/training/train_gtauav.py::TrainConfigGTAUAV` | 1 класс, 50+ полей | +| Legacy `@gin.configurable` на классе тренировки | `src/training/train.py::TrainConfig` (v2) | 1 класс, 14 полей | +| `@gin.configurable` на не-конфиг классах | `src/losses/multi_infonce.py::InfoNCELoss`, `src/losses/weighted_infonce.py::WeightedInfoNCELoss`, `src/datasets/visloc_with_captions.py::GeoLocCaptionDataset` | 3 класса | +| Локальный `@dataclass` для модели (без gin) | `src/models/sofia_v1/config.py::SOFIAv1Config` | 1 класс | +| Module-level пути | `src/training/train_gtauav.py`, `src/datasets/gtauav_dataset.py`, `scripts/make_split.py`, `scripts/filter_segmentation.py` | 4 файла | +| `argparse` | `src/training/train_gtauav.py`, `src/training/train.py`, `scripts/make_split.py`, `scripts/filter_segmentation.py` | 4 файла | + +--- + +## Несоответствия стандарту + +### 1. `@gin.configurable + @dataclass` — критическое нарушение + +**Правило** (`Стандарт_написания_кода_для_DL_CV.md` §3.1, `Reference Examples → Anti-patterns`): + +> «**Запрещено** использовать `dataclass` совместно с gin» + +**Что нарушает:** + +`src/training/train_gtauav.py`: +```python +@gin.configurable(module="src.training.train_gtauav") +@dataclass # ← FORBIDDEN +class TrainConfigGTAUAV: + train_json: str = _TRAIN_JSON + test_json: str = _TEST_JSON + rgb_root: str = _RGB_ROOT + # ... 50+ полей всего: + # пути к данным, к моделям, training schedule, model arch (mona, gates, + # baseline, stripnet, asymmetric), loss params, sampler params, + # tracking flags (wandb/tb/gradcam/profiler) +``` + +**Почему это нарушение:** `@dataclass` авто-генерирует `__init__`, в который gin **тоже** инъектирует параметры. Получается двойная магия: dataclass читает type hints для генерации сигнатуры, gin читает гин-биндинги для подмены значений. На практике работает, но это именно та комбинация, которую стандарт запрещает. + +**Масштаб:** **1 класс**, но он держит **всё** — путь, hardware, model, loss, sampler, tracking. Это блокирует разделение конфига на оси. + +--- + +### 2. `@gin.configurable` на не-конфиг классах — критическое нарушение + +**Правило** (стандарт §3.1): + +> «Декоратор `@gin.configurable` **только на классах** [конфигурации, *не на бизнес-логике*]» + +**Что нарушает:** + +| Файл | Класс | Параметры (которые проникают в `.gin`) | +|---|---|---| +| `src/losses/multi_infonce.py` | `InfoNCELoss` | `temperature_init`, `temperature_final`, `label_smoothing`, `weight_q2g`, `weight_g2q`, `learnable_temperature`, `tau_min`, `tau_max`, `hard_mining_k` | +| `src/losses/weighted_infonce.py` | `WeightedInfoNCELoss` | `temperature_init`, `learnable_temperature`, `label_smoothing`, `k`, `tau_min`, `tau_max` | +| `src/datasets/visloc_with_captions.py` (v2) | `GeoLocCaptionDataset` | `query_file`, `data_root`, `image_transform`, `drop_caption_prob`, `seed` | + +**Почему это нарушение:** конфиг и бизнес-логика — разные слои. Когда `nn.Module` или `Dataset` декорированы `@gin.configurable`, гин лезет в **их** `__init__` помимо лезения в `TrainConfigGTAUAV.__init__`. В `gtauav_balanced.gin` это видно прямо: + +```gin +# Параметры дублируются — раз в TrainConfigGTAUAV, раз в InfoNCELoss: +TrainConfigGTAUAV.tau_init = 0.07 +TrainConfigGTAUAV.label_smoothing = 0.1 +TrainConfigGTAUAV.weight_q2g = 0.6 +TrainConfigGTAUAV.weight_g2q = 0.4 + +# ---- InfoNCELoss (gin-configurable) ---- +InfoNCELoss.temperature_init = 0.07 # ← дубль tau_init +InfoNCELoss.label_smoothing = 0.1 # ← дубль +InfoNCELoss.weight_q2g = 0.6 # ← дубль +InfoNCELoss.weight_g2q = 0.4 # ← дубль +InfoNCELoss.tau_min = 0.01 +InfoNCELoss.tau_max = 0.1 +InfoNCELoss.hard_mining_k = 0 +``` + +**Это активный источник тихих багов:** если кто-то поменяет `TrainConfigGTAUAV.tau_init = 0.05`, а `InfoNCELoss.temperature_init` забудет — обучение пойдёт с `0.05` в логике trainer-а и `0.07` в самой loss-функции. Никаких ошибок не будет, метрики просто будут странными. + +**Масштаб:** **3 класса**, но самый болезненный — `InfoNCELoss`, потому что у него **самая большая зона перекрытия** с `TrainConfigGTAUAV`. + +--- + +### 3. `argparse` — критическое нарушение + +**Правило** (стандарт §3.4): + +> «**Запрещён argparse** — все параметры из .gin файлов» + +**Что нарушает:** + +| Файл | Кол-во CLI флагов | Самые проблемные | +|---|---|---| +| `src/training/train_gtauav.py::main` | ~15 | `--config`, `--baseline`, `--batch-size`, `--epochs`, `--filter-meta`, `--wandb`, `--gradcam`, `--profile`, `--gin-param`, `--resume`, `--output-dir` | +| `src/training/train.py::main` (v2) | 1 | `--config` | +| `scripts/make_split.py::main` | 3 | `--ratio`, `--seed`, `--output-dir` | +| `scripts/filter_segmentation.py::main` | 3 | `--segm-root`, `--threshold`, `--output` | + +**Самый ядовитый паттерн** в `train_gtauav.py`: + +```python +parser.add_argument("--gin-param", nargs="*", help="Override gin params from CLI") +# ... +gin.parse_config_files_and_bindings([cfg_file], extra_bindings) +``` + +CLI **перекрывает** gin-биндинги. Это создаёт **3 источника правды** на один и тот же параметр: дефолт в `__init__`, значение в `.gin`, значение в `--gin-param`. Какое из них применилось в конкретном запуске — невозможно установить иначе как чтением логов. + +**Масштаб:** **тренировочный код** (`train_gtauav.py`) — критично; **скрипты** (`make_split.py`, `filter_segmentation.py`) — пограничный случай (см. ниже §7). + +--- + +### 4. `include` для композиции `.gin` — пограничный случай + +**Правило** (`REQUIREMENTS_GIN_STYLE.md` §8): + +> «Использовать **минимальный** набор возможностей gin: `@gin.configurable`, `gin.parse_config_file()`. Нет: `gin.register()`, `gin.constant()`, `gin.query_parameter()`, **макросы, ссылки между конфигами**» + +**Что есть сейчас:** + +```gin +# conf/gtauav_balanced_asym.gin +include 'conf/gtauav_balanced.gin' + +TrainConfigGTAUAV.shared_encoder = False +TrainConfigGTAUAV.mona_last_n_blocks = 24 +TrainConfigGTAUAV.output_dir = "out/gtauav/balanced_asym" +``` + +`include` — это **встроенная фича gin** для композиции. Формально она: +- ✅ Не входит в явный список запрещённого (`gin.constant`, `gin.register`, `gin.query_parameter`, макросы) +- ❌ Попадает под формулировку «**ссылки между конфигами**» в `REQUIREMENTS_GIN_STYLE.md` §8 + +**По букве правил это нарушение.** По духу — `include` решает реальную проблему (DRY), но создаёт неявную зависимость: чтобы понять, с какими параметрами запускается `gtauav_balanced_stripnet_unfrozen.gin`, нужно прочитать **3 файла** (`unfrozen` → `stripnet` → `balanced`). + +**Масштаб:** все 14 `gtauav_*.gin` (кроме `gtauav_balanced.gin`, который сам — корень дерева) и v2 (`baseline_no_text.gin`, `text_heavy.gin` тянут `balanced.gin`). + +--- + +### 5. Один мега-конфиг на всё — не «жёсткое» нарушение, но против духа + +**Правило** (стандарт §3.3, `Рекомендуемые_gin-config_категории.md`): + +> «Каждый `.gin` → один конфиг-класс» +> +> «Принцип разделения: если два параметра меняются **вместе** — в одном конфиге. Если **независимо** — в разных» + +**Что есть сейчас:** + +`TrainConfigGTAUAV` миксует **несколько независимых осей изменчивости** в один класс: +- Пути к данным/моделям (меняются при смене машины) ↔ training schedule (меняются при смене эксперимента) ↔ model arch (`baseline_mode`, `shared_encoder`, `mona_*`, `stripnet_*`) ↔ loss (`tau_init`, `label_smoothing`, `weight_q2g`, ...) ↔ sampler (`sampler_type`, `dss_*`) ↔ tracking (`use_wandb`, `use_tb`, `use_gradcam`, `use_profiler`) + +В одном `.gin` лежат биндинги для всех этих осей. Когда нужно «то же обучение, но без wandb» — приходится копировать целый `.gin` файл. + +**Это не нарушение явного запрета, но прямое следствие нарушения #1:** пока есть один большой `@dataclass + @gin.configurable`, иначе расположить параметры просто негде. + +**Масштаб:** проникает во все 12 `gtauav_*.gin` файлов одинаково. + +--- + +### 6. Module-level хардкод путей — нарушение + +**Правило** (стандарт §6 чеклист): + +> «Нет захардкоженных model ID / промптов / размеров?» + +**Что нарушает:** + +```python +# src/training/train_gtauav.py (module level) +_RGB_ROOT = "/home/servml/Документы/datasets/GTA-UAV-LR" +_CAPTION_ROOT = "/home/servml/Документы/datasets/GTA-UAV-LR-captions" +_TRAIN_JSON = "meta/train_80.json" +_TEST_JSON = "meta/test_20.json" +_DINO_WEB = "nn_models/DINO_WEB/dinov3-vitl16-pretrain-lvd1689m.pth" +_DINO_SAT = "nn_models/DINO_SAT/model.safetensors" +_LRSCLIP = "nn_models/LRSCLIP/DGTRS-CLIP-ViT-L-14.pt" + +# src/datasets/gtauav_dataset.py (module level) — ДУБЛЬ: +_RGB_ROOT = Path("/home/servml/Документы/datasets/GTA-UAV-LR") +_CAPTION_ROOT = Path("/home/servml/Документы/datasets/GTA-UAV-LR-captions") + +# scripts/make_split.py (module level): +_RGB_ROOT = Path("/home/servml/Документы/datasets/GTA-UAV-LR") + +# scripts/filter_segmentation.py (module level): +SEGM_ROOT = Path("/home/servml/Документы/datasets/GTA-UAV-LR-aug/segm") +``` + +**Главная проблема — не сам факт хардкода**, а то, что один и тот же путь `/home/servml/Документы/datasets/GTA-UAV-LR` дублируется в **трёх** местах: train_gtauav.py + gtauav_dataset.py + make_split.py. Если переехать на другую машину — нужно править 3 файла; если забыть один — silent breakage. + +**Масштаб:** 4 файла, ~10 module-level констант пути. + +--- + +### 7. Скрипты `make_split.py` / `filter_segmentation.py` — спорный случай + +**Что есть:** +- `argparse` (3 параметра в каждом) +- module-level пути (`_RGB_ROOT`, `SEGM_ROOT`) +- Запускаются однократно перед тренировкой +- Не интегрированы в `Trainer.run()` + +**Применять ли стандарт «нет argparse» к ним?** + +Стандарт направлен против `argparse` в **тренировочном коде**, где параметров 50+ и нужна воспроизводимость. У `make_split.py` 3 параметра, и это **разовая препроцессинг-утилита**. Здесь буква и дух стандарта расходятся. + +**Это не «несоответствие, требующее немедленной правки»**, а **открытый вопрос для следующего шага**: применять ли gin-стиль к препроцессинг-скриптам или оставить argparse. + +--- + +### 8. Sofia models — `@dataclass` без gin (формально не нарушение) + +**Что есть:** + +```python +# src/models/sofia_v1/config.py +@dataclass +class SOFIAv1Config: + variant: Literal["tiny_tiny", "tiny", "small", "small_v2"] = "small" + in_channels: int = 3 + input_size: int = 256 + dcn_variant: Literal["v2", "v4"] = "v2" + d_descriptor: int = 1024 + use_film_altitude: bool = True + altitude_norm: float = 500.0 + use_text_film_uav: bool = True + use_text_film_sat: bool = True + text_film_dim: int = 1024 + text_film_hidden: int = 256 +``` + +**Стандарт** запрещает **`@gin.configurable + @dataclass`**, но не запрещает `@dataclass` сам по себе. `SOFIAv1Config` без gin — формально стандарт **не нарушает**. + +**Однако:** в `gtauav_balanced_sofia*.gin` (по скриншоту локально есть, в репо пока нет) параметры sofia, очевидно, прокидываются в `TrainConfigGTAUAV` как обычные поля. То есть sofia сейчас живёт в **двух** местах: +- `SOFIAv1Config` (dataclass, в коде модели) +- `TrainConfigGTAUAV.sofia_*` (если там такие есть) или только через позиционное создание `SOFIAv1Config()` в `Trainer` + +Это не нарушение, но **архитектурный раскол**: dataclass-конфиги внутри модельных подсистем + gin-конфиги снаружи. + +**Это тоже открытый вопрос для следующего шага**, не текущая проблема. + +--- + +### 9. Прочее по чеклисту стандарта (мелочи) + +| Пункт | Состояние | Файлы | +|---|---|---| +| `from __future__ import annotations` первой строкой | ✅ есть в ключевых файлах | проверено в `train_gtauav.py`, `gtauav_dataset.py`, `multi_infonce.py`, `weighted_infonce.py`, `train.py` | +| Строгие type hints | ✅ в основном | пара мест с `dict` без параметров (`_atomic_save(obj: dict)`) | +| Google-style docstrings | ✅ есть, качество хорошее | — | +| `@torch.inference_mode()` вместо `@torch.no_grad()` | ❌ используется `@torch.no_grad()` | `train_gtauav.py::_evaluate`, `_embed_drone_queries` | +| Atomic writes | ⚠️ есть, но без cleanup на ошибке | `train_gtauav.py::_atomic_save` (нет `try/except` — `.tmp` остаётся при сбое) | +| Английский в коде/комментариях | ✅ есть | — | +| Импорты stdlib → third-party → local | ✅ есть | — | + +--- + +## Сводка — что нарушено и насколько срочно + +| # | Нарушение | Срочность | Зона воздействия | +|---|---|---|---| +| 1 | `@gin.configurable + @dataclass` на `TrainConfigGTAUAV` | 🔴 критично | блокирует всё остальное | +| 2 | `@gin.configurable` на `InfoNCELoss`, `WeightedInfoNCELoss`, `GeoLocCaptionDataset` | 🔴 критично | активный источник тихих багов | +| 3 | `argparse` в тренировочном коде | 🔴 критично | три источника правды на параметр | +| 4 | `include` между `.gin` файлами | 🟡 пограничный | формально нарушает «нет ссылок между конфигами» | +| 5 | Один мега-конфиг (нет разделения на оси) | 🟡 следствие #1 | разрешится с #1 | +| 6 | Module-level пути в 4 файлах | 🟡 нарушение, но не критично | дубли — реальная проблема | +| 7 | `argparse` в скриптах препроцессинга | ⚪ открытый вопрос | спорный случай | +| 8 | `@dataclass` в `SOFIAv1Config` (без gin) | ⚪ открытый вопрос | формально не нарушение | +| 9 | `@torch.no_grad()` вместо `@torch.inference_mode()` | 🟢 мелочь | косметика | +| 10 | `_atomic_save` без cleanup .tmp | 🟢 мелочь | редкое последствие | + +--- + +## Что предлагаю обсудить дальше (Шаг 2) + +Прежде чем двигать код, нужно принять решения по 4 развилкам: + +1. **`include` между `.gin`** — терпим как удобный DRY-механизм или приводим к плоской иерархии (каждый эксперимент = самодостаточный набор `.gin` без `include`)? + +2. **Разделение `TrainConfigGTAUAV` на классы** — на сколько и по каким осям? + - Вариант **5 классов** (Pipeline / Hardware / Models / Training / Tracking) из «`Рекомендуемые_gin-config_категории.md`». + - Вариант **6 классов** (отдельный Loss + Sampler). + - Вариант **3 класса** (всё, что было — в Training, плюс Pipeline + Tracking). + +3. **Sofia + dataclass** — оставлять `SOFIAv1Config` как dataclass-структуру внутри модели или переписать в обычный класс/влить в `ModelsConfig`? + +4. **Скрипты** — переводить на gin или оставить argparse? + +После решения этих 4 пунктов план рефакторинга становится однозначным. \ No newline at end of file