[Bug] Fix Infinite Loop Caused When Calling Methods Not Overrided In PyClass

[zxybazh]

This PR Introduced a decorator for PyClass methods to avoid inifinite loop when calling a method not overrided.
E.g. PySpaceGenerator().generate_design_space(mod) would now throw a NotImplementedError instead of a segmentation fault. Correspondingly, any new class inherited from PyClass would have to override all the classes, otherwise would cause a NotImplementedError when constructing it.

[zxybazh]

1/4) One interesting finding is that, if we think it through, we cannot define a python class that partially use c++-side implementation as we was assuming in PyTaskScheduler because any function not overrided, would call ffi, which calls back to the python side implementation, making a infinite loop.

[zxybazh]

2/4) Therefore, in our current design, it's legit to force override of all methods to avoid infinite loop. Of course, we cannot avoid infinite loop caused by calling ffi back, that would be user's responsibility though.

[zxybazh]

3/4) Now let's take a look back to the problem of how to correcly deal with the PyClasses. They are inherited from the base classes, so any ffi call back to its base class will invoke its own function, which calls back to the python side packed func. If we want to cut the evil loop, we need to do a check on the c++ side, if the PackedFunc simply calls itself, we need to call the base classes' corresponding method, or throw not implemented.

[zxybazh]

4/4) I don't have a very good idea to have it implemented, while it's feasible. Would like to know how do you think about the problem.

[junrushao]

Thank you @zxybazh for spotting this very subtle bug!! The PR looks good to me overall. Would you mind update the TaskScheduler to allow using parent classes' virtual methods? Then this PR will be good to go :-)

[zxybazh]

Hi @junrushao1994 , there are some major updates here:

1. Update the init function for PyTaskScheduler.
2. Changed all PyClasses to optional override, as per our discussion.
   • Virtual functions are checked for nullptr.
   • Other functions are directed to base class.
3. Added and enhanced unittests.

Thanks for the review and discussion, and please let me know if this looks good to you.
Now the task scheduler is much more usable, as shown in the test case.

An example of calling route of _is_task_running if not overrided in Python:

Python side:

• PyTaskScheduler does not have _is_task_running
• Call TaskScheduler's is_task_running, which calls ffi

C++ side:

• The ffi calls TaskScheduler's is_task_running
• But it is overrided in PyTaskScheduler
• PyTaskScheduler checks if the function is overrided in python
  - If not, it returns the TaskScheduler's vtable, calling
  TaskScheduler::IsTaskRunning

[zxybazh]

The errors are now raised as NotImplementError when declaring the class. And the CHECKs in c++ are changed into ICHECK without error type (because it's internal error, it should throw a NIE beforehand). Tests passed and ready for review.

[zxybazh]

Another change is next_task_id func's underscore is removed, also tune as in TaskScheduler. That is because, tune is used for function calling and next_task_id is a function that you have to override for TaskScheduler.

[junrushao]

@zxybazh Thanks for the update! The only issue I'm seeing right now is that we don't need to defer CHECK to method invocation time. For example, the nullability check of PyMutator could be checked the method Mutator::PyMutator

[zxybazh]

@zxybazh Thanks for the update! The only issue I'm seeing right now is that we don't need to defer CHECK to method invocation time. For example, the nullability check of PyMutator could be checked the method Mutator::PyMutator

Thanks for reviewing. The nullability check has been done when calling "check_override" so during invocation it's just double checking, thus using ICHECK.

[zxybazh]

Therefore, when instantiating any object without all required methods overrided, it will raise a not implemented error.

[junrushao]

Okay that sounds great! Please feel free to merge this PR after my nitpick is addressed

[junrushao]

@zxybazh would you also send this PR to apache mainline? Thanks a lot!

[zxybazh]

Sure will do after I finish design docs.

…

On Thu, Nov 4, 2021 at 5:13 PM Junru Shao ***@***.***> wrote: @zxybazh <https://github.com/zxybazh> would you also send this PR to apache mainline? Thanks a lot! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#496 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYOAZWTYWKUFVX2SY2ER2LUKMVSLANCNFSM5G6A5WQQ> .