Adelionek
commented
1 year ago Dzięki za tak porządne review. Szczerze nie spodziewałem się, że aż tak się do tego przyłożysz :). Nie sądziłem też, że tych nowych zmian jest aż tyle, chyba lekko straciłem rachubę.
Faktycznie, jak przejrzałem jeszcze raz całego PR'a to jest dużo zmian 'z dupy' które zostały zrobione 'po drodze'. Wynikały one głównie z tego, że po ostatnim CR dodałem lokalnie dużo rzeczy, które były kluczowe do osiągnięcia efektów które mamy już wdrożone np. augmentacja, konwersja modelu do onnx itp. Totalnie się zgadzam, zrobił się syf i jest to totalnie nie wg procesu, kolejne będą mniejsze. Jednak mój plan był trochę inny. Chciałem dopiąć tego PR'a żeby wszystko co jest wdrożone było na mainie i później dopiero do tego porobić poprawki. Jest wiele podejść do tworzenia kodu, ja jestem zwolennikiem najpierw napisania działającego kodu a w kolejnym kroku 'upiększanie' go.
ad Dostosowanie istniejącego kodu do nowych zmian
Na początku mieliśmy podejście 1 jupyter 1 model. Przy tak dużej ilości zmian jak mlflow, augmentacja, pylint ciężko jest trzymać je spójne, stąd przeniesienie kodu do plików pythonowych. Jak dla mnie to jupytery możemy albo wywalić z repo albo dodać je do gitignore i każdy niech sobie dostosowuje do swoich eksperymentów. Wszystkie pliki pythonowe w tym PR były up to date do wszystkich wprowadzonych zmian.
ad N pieczeni na 1 ogniu
Im sorrry. Już trochę odniosłem się do tego na wstępie. Docelowo będę o tym pamietać i będzie to bardziej granularne. Jednak musimy znaleźć jakiś złoty środek między jakością kodu a jego efektywnością. Podejście w którym robimy merge do maina raz na miesiąc bo próbujemy przy każdym mergu mieć idealnie uporządkowany kod nie jest efektywne. Szczególnie, że i tak mergujemy kod do maina wiedząc, że w przyszłości będzie musiał być on jeszcze zrefactorowany. Brakuje mi tu trochę gitflow i dodatkowego brancha np dev/qa do którego byśmy wrzucali mniej dopracowane zmiany (ale działające). Wtedy moglibyśmy ustalić, że to co trafia na dev/qa jest już odwzorowane na azure/froncie/mlflow a to co trafia do maina jest już docelowym rozwiązaniem które jest zrefactorowane (generyczny, uporzątkowany kod itp). Brakuje nam jedynie dodatkowego środowiska na Azure ale w teorii można to ogarnąć. Przykład z augmentacją jest bardzo trafny. Powinno być to zrobione na osobym branchu. Tak samo jak wprowadzenie configu.
ad Uwspólnienie kodu
Jak najbardziej jestem za. Tylko co jest ważniejsze? Zebranie w końcu całego działającego kodu w całość i następnie zajęcie się naprawianiem/abstrachowaniem kodu, czy robienie tego upfront? Myślę, że powinniśmy korzystać z plików pythonowych, tak jak wcześniej wspomniałem.
ad Pylint
Jeśli o to chodzi to szczerze nie wiedziałem, że powodem jest plik init.py. Pylint rzucał błędy gdy importowałem biblioteki związane z azure.
ad Jupytery
Zhardcodowane ścieżki wyrzucę, były użyte tylko w ostatnim kroku do 'sample predict image' gdzie podawałem ścieżkę do lokalnego zdjęcia i modelu. Wywale to. Przerzucamy się na pliki pythonowe.
Tak jak napisałem na telegramie, chyba będzie sensowniej jak to rozbije to na mniejsze PR'y co sam zaproponowałeś.
Trochę dużo się tutaj zadziało. Następne powinny być definitywnie mniejsze.