App: Raise a better error when `run` method not defined for the `LightningWork`

[krshrimali]

🚀 Refactor

When a user forgets to define a run method in their LightningWork, a TypeError is raised:

Your Lightning App is starting. This won't take long.
ERROR: Found an exception when loading your application from bug.py. Please, resolve it to run your app.

Traceback (most recent call last):
  File "bug.py", line 19, in <module>
    app = L.LightningApp(Flow())
  File "bug.py", line 13, in __init__
    self.w = Work()
TypeError: Can't instantiate abstract class Work with abstract method run

Pitch

We should raise a better warning, pointing the users to the documentation on how to implement the run method.

Alternatives

Is there a reason why LightningWork is an abstract class? Can we make it a normal class like LightningFlow, and raise a NotImplementedError in the run method instead? (Note that LightningFlow is not an abstract class)

Additional context

As reported by @williamFalcon internally.

---

If you enjoy Lightning, check out our other projects! ⚡

• Metrics: Machine learning metrics for distributed, scalable PyTorch applications.

• Lite: enables pure PyTorch users to scale their existing code on any kind of device while retaining full control over their own loops and optimization logic.

• Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, fine-tuning, and solving problems with deep learning.

• Bolts: Pretrained SOTA Deep Learning models, callbacks, and more for research and production with PyTorch Lightning and PyTorch.

• Lightning Transformers: Flexible interface for high-performance research using SOTA Transformers leveraging PyTorch Lightning, Transformers, and Hydra.

cc: @tchaton @hhsecond @Borda @ethanwharris @awaelchli

cc @justusschock @awaelchli @rohitgr7

[awaelchli]

Is there a reason why LightningWork is an abstract class?

A LightningWork is not useful if run is not implemented, and we can't have a default implementation. The class is marked abstract to signal to the user they need to implement run(). This marks the contract between user and framework: The user can subclass LightningWork and implements a valid run method. Then the framework can remotely execute the code in run().

Note that LightningFlow is not an abstract class)

There, the contract is different: The LightningApp class must call root_flow.run() at the root level, hence user needs to implement run for the LightningFlow at the root. But the children can have arbitrary methods, and the user is responsible for calling them in their tree of flows. One could make this stricter by marking run on the flow abstract, to signal a clear contract to the user.

We should raise a better warning, pointing the users to the documentation on how to implement the run method.

You can change the abstract method to one raising NotImplementedError, but IMO the static way is much better since it forces as stronger contract and lets IDE's help you auto-complete the implementation directly, without you having to go to the docs learning "how to implement the run method".

I'm ok with either way, as long as we force the contract.

[krshrimali]

Thanks @awaelchli - I don't disagree to anything you said. Tried going the NotImplementedError way, but that didn't work. I was missing something I guess.

But anyways, a question - do you think we can modify the error message without changing anything in the API/contract? :) Just asking for knowledge if it's possible at all.

[tchaton]

Hey @krshrimali Yes, you could override the Type class and override the error message.

[williamFalcon]

yes, the error message needs to be friendly... i've brought this up multiple times. 99% of python developers probably don't even know what "abstract class" means, nor should they... a message in plain HUMAN english is our goal... "you didn't implement the run method in your XYZ class. This is required. example:

class ...

   def run(self): # <------- this guy
      # TODO: add your own code here

[awaelchli]

Taking this one off your shoulders @krshrimali. See the two attached PRs 😃