fastai / fastbook

The fastai book, published as Jupyter Notebooks
Other
21.67k stars 8.37k forks source link

Suggestion: replace `import *` with named imports #526

Open davidgilbertson opened 2 years ago

davidgilbertson commented 2 years ago

I've just started the course and I'm finding that the use of import * makes learning more difficult than it should be. I don't know what functions are coming from where, and live in fear that I'll create a variable and accidentally overwrite something that's used elsewhere on the page.

In general, this practice is discouraged, says the Python docs: "The practice of importing * from a module or package is frowned upon, since it often causes poorly readable code".

I'm just one opinion, but so far my impression of the fastai code snippets in the course is that they are indeed poorly readable, I would rather burn mental CPU cycles on understanding the concepts, not deciphering code.

BTW, I am aware of the shift+tab shortcut in Jupyter, and am aware I can type a function name and run a cell to see where it was defined, but that only half-solves half the problem.

One of the many benefits to doing something like import fastai.vision as fav is that you get scoped auto-complete. So for those of us who haven't yet completely memorised every single function in the library, we can do fav.<tab> to jog our memory when looking for a function that we half-remember. When you import blah as * you blow away this scoping which isn't just a problem for serious software projects, it's also a pain when exploring and learning.

davidgilbertson commented 2 years ago

Two seconds after logging this, I returned to the course, ran a cell, and got this: image

A slightly more rare example of why import * is poor form: this is quite difficult to troubleshoot, given that I have no idea where it's supposed to have come from, certainly more difficult than it should be.

Edit, by the way, this was caused by somehow fastai 2.5.3 being installed in the Kaggle notebook. I had to run the install again and use importlib.reload() to get it back to using 2.7.9 and able to see vision_learner

leszekhanusz commented 2 years ago

Yes please, it is sometimes very confusing when you encounter some module, class or method which is not found above in the code.

Abyszero commented 2 years ago

Although I strongly agree, this has been an issue from the very beginning and has been addressed during the MOOC in the past. It's not going to get fixed until there's a change of ownership. Best I can suggest is to rely on the docs instead of IDE code completion, and get to know the the library as well as you can.

davidgilbertson commented 2 years ago

It's weird, everything else about the course strikes me as very well thought out with clever ways to explain complex concepts. But then the code is littered with this sloppy and obfuscating import * habit that the rest of the community has worked out is a bad idea. Frustrating!

Another concrete example from chapter 4 of the book. If you were to do

from torch import *
from fastai.vision.all import *

tensor(Image.open(some_path))

It would work fine. But the following (same code, different import order) would throw an error, because fastai modifies tensor, and whichever import comes last, wins.

from fastai.vision.all import *
from torch import *

tensor(Image.open(some_path))

So the book is teaching certain behaviour of tensor, but actually it's not something that will work with PyTorch tensors on their own, and it's all obscured by import *. To show this as fa.tensor() would be so much clearer.

programmer-ke commented 1 year ago

As I software engineer getting into ML, I also noticed that notebooks tend to do * imports and initially found it odd (since in typical python coding in the software engineering context we tend to want to show where an import came from).

However, I've come to see it as some sort of convention in Data Science/ML notebooks that simplifies typing.

One thing that can help in knowing where something came from is checking the __module__ attribute e.g. vision_learner.__module__.