Add new pre_refactor analysis.md
This commit is contained in:
pikaliov
297
pre_refactor_analysis.md
Normal file
297
pre_refactor_analysis.md
Normal file
@@ -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 пунктов план рефакторинга становится однозначным.
|
||||||
Reference in New Issue
Block a user