collab-uniba / pynblint

Pynblint is a linter for Python Jupyter notebooks.
MIT License
40 stars 2 forks source link

Code extraction #1

Closed RomitoVincenzo closed 3 years ago

RomitoVincenzo commented 3 years ago

Nel branch codeExtraction puoi trovare diverse funzioni di estrazione del codice Pyton e numerazione delle definizioni di funzione presenti così come mi avevi chiesto. Ho esplorato sia la soluzione con la deserializzazione, sia quella NBconvert e sia quella di parse tree. Sono 2-3 alternative che fanno la stessa cosa, le ho sviluppate e commentate in modo che potessi dare una occhiata e magari decidere quale fosse l'approccio migliore. I nomi delle funzioni sono stati scritti in modo da far comprendere quale fosse l'implementazione, per agevolare il tuo lavoro nella review, ovviamente nel momento in cui si decide quale implementazione si vuole utilizzare tra quelle che ho sviluppato si andranno a modificare gli identificatori per renderli più chiari e aprirei una pull request per il merge. Se hai bisogno di discuterle sono a tua disposizione.

louieQ commented 3 years ago

Ciao @RomitoVincenzo,

ti segnalo alcuni miglioramenti necessari e ti suggerisco qualche modifica da fare.

In linea generale, vorrei sapere da te se ritieni che ci sia qualche vantaggio nell'usare nbconvert. Se non ne trovi, potremmo analizzare direttamente i sorgenti in formato .json (credo sia il metodo che garantisce la massima flessibilità, anche nell'ottica di esaminare il markdown e altri eventuali tipi di celle).

Miglioramenti necessari

Dichiarazione delle dipendenze di progetto

Hai fatto bene a dichiarare le dipendenze del progetto nel file testEnv.yml; tuttavia mi sembra che tu stia lavorando in un ambiente anaconda che contiene tantissime librerie in più rispetto a quelle effettivamente usate nel tuo notebook. Questa cosa non va bene: se io volessi riprodurre il tuo ambiente d'esecuzione sulla mia macchina, dovrei installare giga e giga di dipendenze inutilmente.

Dovresti creare un nuovo ambiente - assicurandoti di installarvi solo lo stretto necessario - e, a partire da questo ambiente "pulito", ricreare il file testEnv.yml con la dichiarazione delle dipendenze reali.

Attieniti al "Single Responsibility Principle"

Ogni funzione dovrebbe assolvere uno e un solo compito.

Una funzione come FunctionNumberNbTree, che analizza il codice Python per trovare il numero di definizioni di funzione ivi contenute, dovrebbe ricevere in input una stringa con il codice da analizzare ed emettere in output il risultato. La lettura del file .ipynb e la relativa conversione vanno fatte all'esterno, per mezzo di altre funzioni.

Si possono sempre costruire funzioni più astratte che eseguono in pipeline le funzioni elementari, ma al livello a cui siamo non è ancora il caso. Queste che hai definito sono tutte funzioni che dovrebbero avere una singola responsabilità.

Suggerimenti di minore importanza

louieQ commented 3 years ago

@RomitoVincenzo dopo aver selezionato le implementazioni che ti convincono di più, trasferiscile all'interno di un modulo Python da creare nella stessa cartella in cui sono presenti i notebook (ad es.: utils.py).

All'interno del notebook potrai importare il modulo:

import utils

e invocare tutte le funzioni che vi avrai definito:

utils.FunctionNumberNbTree('filename.ipynb')

Approfitto a ricordarti che gli identificatori di funzione in Python hanno sempre l'iniziale minuscola. L'iniziale maiuscola è riservata agli identificatori di classe.

louieQ commented 3 years ago

@RomitoVincenzo continua pure a lavorare e pushare commit su questo branch; menzionami in un commento quando sei pronto a richiedermi una nuova revisione.

RomitoVincenzo commented 3 years ago

@louieQ

Implementazione scelta

Ho deciso di lasciare la semplice implementazione con metodo load() e non con nbConvert in quanto credo che ci lasci più libertà, consentendoci di lavorare con il notebook in formato JSON.

Dipendenze di progetto

Ho create un nuovo environment, specificando ''--no-default-packages'' in modo che non importi pacchetti superflui. Ho unicamente installato il package "notebook" e "ipython kernel" per la gestione del kernel nel virtual environment. Dovrebbe andare meglio rispetto al precedente yml.

Modulo py

Ho creato il modulo utils come suggerito e importato sul notebook di prova.

.gitignore

Ho creato opportunamente il file .gitignore ma i checkopoint e la cache del modulo python continuano ad esistere. Forse mi sta sfuggendo qualcosa


Dovrei aver seguito tutti gli appunti che mi hai fatto, l'unico dubbio che rimane è quello del file .gitignore che sembra non funzionare

RomitoVincenzo commented 3 years ago

@louieQ ho lavorato sugli accorgimenti che mi hai sottolineato. L'unica problematica è sul parse tree del codice python ridotto a stringa: se ho capito bene si interrompe perchè trova un errore di indentazione che su una cella del notebook non da come errore, ma visto nell'insieme come codice python segnala giustamente errore. Ovvero, se una cella di un notebook target contiene solo una istruzione, quest'ultima potrebbe essere arbitrariamente indentata e il notebook non da alcun tipo di errore, contrariamente genera un errore quando si va a creare il parse tree. Ho pensato dunque, di ritornare alla soluzione nbconvert che non presenta questa problematica, credo perchè corregga l'indentazione "sbagliata" nella conversione de notebook a file .py. Ho deciso di lasciare comunque il metodo notebookToCode() con Load nel caso ci servisse. In ogni modo ora è tutto funzionante. Ti posto qui di seguito uno screen dell'errore, così puoi dare un'occhiata anche tu. Credo si possa proseguire con il merge. La cella erratamente indentata che genera l'errore è quella nel notebook TargetNotebooks/my-attempt-at-analytics-vidhya-job-a-thon.ipynb con execution_count di 50. cattura