fal-ai / fal

⚡ Fastest way to serve open source ML models to millions
https://fal.ai/docs
Apache License 2.0
549 stars 47 forks source link

forbid nested isolated functions #169

Closed efiop closed 3 weeks ago

efiop commented 7 months ago

If one tries to call one isolated function from another isolated function we currently might fail with a deserialization error https://github.com/fal-ai/fal/blob/8a519b156c7786e994571f882df134ebf12e83e8/projects/fal/tests/test_stability.py#L30 , but depending on how pickling goes we might fail to serialize it in the first place. It would be nice to just raise a proper exception instead. E.g. we could utilize a global flag to raise when an isolated function is trying to to run another isolated function, or we might go the code inspection route to hunt for IsolatedFunction instances in the function code, or we could also do something on the isolate server side to forbid nesting.

Clarification: we probably don't want to limit someone using isolated function in another library, but we definitely want to limit isolated functions in pickled code.

Related https://github.com/fal-ai/fal/pull/168

linear[bot] commented 7 months ago

FEA-2286 forbid nested isolated functions

squat commented 7 months ago

This kind of opens up a can of worms. What if the isolated function is part of a library and the user of the library isolates their their app code?

chamini2 commented 7 months ago

I think if we do not want to support it, we can just document it clearly.

If we want it to fail in runtime, there is an env var we set in agents that is something like ISOLATE_AGENT=1 (name may be slightly different). We can just check for that before running a function and raise.

But I would be against forbidding by doing code analysis because it is hard and not really necessary.

My take is we can say it is not officially supported but use at your own risk.

efiop commented 7 months ago

@squat Good point. That should work. My concern is another isolated function in pickled code, that's what should be forbidden as pickling/unpickling of that might not even work properly. I should've clarified it in the issue itself. Thanks!

efiop commented 7 months ago

@chamini2 I'm not a fan of code analysis either, but it might be justifiable for better user experience. Pickling is hard and when it blows up it is not trivial for most people to figure out what went wrong, so if we can reasonably (and with reasonable effort in terms of maintenance/fragility) handle common mistakes - we should at least consider it. E.g. in case of isolated function in another isolated function, we might be able to catch that early (e.g. with custom pickler that allows only 1 IsolatedFunction to be picked within a context) and spare the user from trying to hunt down lock/rlock/etc pickling errors just to find out that they are trying to do something that is not supported.

efiop commented 7 months ago

Ah, correction: it actually used to work with koldstart https://github.com/fal-ai/fal/pull/166/files , let me take another look and see if I can make it work after all (need a bit more effort with nested pickling). Though it is still a bit risky to do nested pickling like that considering all the fancy threading/etc that we are doing when calling a host, so forbidding might be an option too. In that test we also install an earlier fal version, which makes pickling this extra special 🤌 😅