scanner-research / scanner

Efficient video analysis at scale
https://scanner-research.github.io/
Apache License 2.0
620 stars 108 forks source link

Registering an op twice from Python raises an exception #109

Closed willcrichton closed 5 years ago

willcrichton commented 6 years ago

This isn't necessarily a bug, but it currently manifests as one. For example, pipelines.detect_faces registers a BBoxNMS kernel every time it's called, which means multiple calls to pipelines.detect_faces will fail after the first. There are a few options here:

  1. Have some form of checking if a kernel is registered, and add that check everywhere in the current examples that use kernel registration.
  2. Don't raise an exception, just print a warning.
  3. Instead of adding an explicit check, just catch the ScannerException.

What do you think @apoms?

fpoms commented 6 years ago

The two scenarios we want to differentiate between is:

1) Two Ops/Kernels registering with the same name but with different implementations 2) Re-registering the same Op/Kernel

The first should definitely be an error, but the latter is fine (maybe just a warning). This basically only comes up in the case of Python kernels (since C++ kernels are registered at file scope) so a Python specific solution should be ok.

Two solutions:

willcrichton commented 6 years ago

TODO: have kernel registrations in Python stdlib check if op is registered first

fpoms commented 5 years ago

I haven't seen this issue anymore. I think this was resolved by making python kernels "Temporary", in that they are suffixed with a UUID generated once per python process and thus unique across client sessions.