-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MMCore feature enable/disable switching #394
Comments
Great writeup @marktsuchida!
I think this is a compelling reason for introducing this. Would you see this mechanism as the preferred way of introducing all experimental new features?
I think it would be better to go the route that keeps the code simpler. Multiple Cores as I've seen it is most often used as a hack to compensate for a shortcoming of the core (e.g. multiple cameras with independent buffers), so I don't think we should design around this if it complicates things in the code. I might be able to provide more helpful feedback if needed if once you've gotten to a prototyping stage. But overall I think this is well considered and would be a good addition |
Thanks for the comments, @henrypinkard. I also think that the static function is better -- what we have here is a tradeoff between two different undesirable couplings: one between the different CMMCore instances, and another between the otherwise independent internal components of MMCore. The latter seems worse, since these feature switches really should be a background mechanism, not something we handle explicitly all over the place. The former is not a critical problem because, as I wrote above, all feature switches should be designed so that there is one way to set it to allow all client code to work (possibly with less stringent error checking, etc.). I will work on a PR.
It's hard to say. I think we should think about it in each case to determine if it helps. But I do think it is useful in cases where we want to add new API functions but don't want to commit to a stable interface too early. By having the dynamic switch, we can allow Java and Python code to experiment with the new feature before we commit to it, and without requiring people to build [py]MMCore[J] themselves. It might be nice to trial this process with a relatively small feature before using it on something large (and it's always best to keep new features fine-grained when possible -- it shouldn't be a problem if experimental feature A requires experimental feature B as long as this is documented). I would say that if any released Java or Python code uses an experimental MMCore feature, it should itself have a mechanism to switch on such experimental code (which probably should be off by default while the MMCore feature(s) it depends on remains experimental). That way, when the experimental Core feature changes in an incompatible way, the higher-level library can continue to function correctly as long as you don't enable the experimental code. Similarly, we might find that we have experimental features that require new functions in MMDevice. This is a little more tricky, but can probably be handled in a similar way: devices can declare to the Core that they implement an experimental interface. We still need to handle the device interface version strictly, though, so overly frequent changes to function signatures might be inconvenient. Also, in this case, the method for device adapters to opt in might simply be to override some optional functions. This is all pretty abstract. We can have a more concrete (and perhaps more useful) discussion when the time comes to actually use this mechanism for a particular experimental feature. |
That all sounds great! |
I'd like to propose that we add a function to
CMMCore
:static void enableFeature(const char* featureName, bool enable) throw (CMMError)
that can be used to turn on and off certain behavior in MMCore.
The direct motivation for this is #384: I want to have a way to switch on or off strict error checking in the Core, so that we can give ourselves (and potentially others) some time to fix existing code while also providing a way to enforce the checks on new or already-conforming code. The idea is that after a call to
CMMCore::enableFeature("StrictInitializationChecks", true);
, incorrect usage of uninitialized devices will result in an exception (whereas in the default state the error is merely logged). Eventually (in a new major version of MMCore), the default can be switched totrue
.If
featureName
is not known, this function should throw an exception.I could just add a single-purpose method for this, but I think it might be better to have a general mechanism for this, since there are other things we might use it for:
Providing a migration path for other API changes, especially related to error handling (stricter checks or reporting previously ignored errors).
Providing a way to develop new features (especially complex ones) without forking or branching MMCore. The new feature can be disabled by default until its API is well-vetted and stable, preventing casual users from accidentally using the experimental feature without realizing. Trying to access the new feature without enabling it would generally result in an exception.
What could go wrong (especially over time)
It is definitely possible to abuse this idea and cause a mess.
We definitely want to prevent this from causing mutually incompatible code from being written by us and the community -- requiring different feature settings to run. Features should only be used as a migration strategy with a clear exit planned, not as a permanent configuration facility.
StrictInitializationChecks
example does not have this problem, because disabling the checks allows all valid code to run.Any use case that would involve switching a feature on and off multiple times is probably a really bad idea, because the effect is global. Features should be designed to be switched on or off once and for all during program startup.
In other words, entities that have full control over the application environment, such as MMStudio or pymmcore(-plus), may switch features at program start, but entities that need to work side-by-side with other entities (e.g., acquisition engines or pycro-manager) should not.
Genuine switchable functionality, if ever needed, should not be a global state.
I also do not think it is a great idea to provide features that disable deprecated functions. Usage of deprecated functions is better checked at the source code level. (Deprecated usage of functions are a different question;
StrictInitializationChecks
could be considered an example of that.)Lifecycle of features
Once a feature name is introduced, it should never be removed (lest it be reintroduced with a different meaning). With the example of
StrictInitializationChecks
, what I envision is something like this:The timing at which a feature can be enabled by default and when the ability to disable it can be removed will need to be determined for each feature, case by case, to minimize disruption.
Other considerations
Should we have a corresponding
static bool isFeatureEnabled(const char* featureName) const throw (CMMError)
? Perhaps yes, so that, say, an acquisition engine can disable an experimental feature that depends on an experimental feature of MMCore. As long as it is not used for more routine purposes.Should we have feature names that enable/disable whole categories (e.g.,
StrictChecks
)? Not sure yet; we can wait until we actually have multiple features to deal with. Perhaps it would be allowed to enableStrictChecks
but not to disable them in bulk (because eventually some checks will be permanently enabled).Should this be a non-static member function of
CMMCore
rather than a static one? That would allow features to be switched on in someCMMCore
instances and not others. Given how poorly we support the use of multipleCMMCore
instances, it may not make much of a difference. In terms of implementing features, it is much simpler if we don't have to give all parts of the MMCore code access to the feature switches of theCMMCore
instance through which we are called. And if we follow the rule (above) that every feature should have a setting that supports all code, it shouldn't matter that we don't support switching per instance.Feedback and thoughts welcome!
The text was updated successfully, but these errors were encountered: