inducer / arraycontext

Choose your favorite numpy-workalike!
6 stars 11 forks source link

Add optional `allow_single_version` Boolean arg to `actx.compile` #227

Open inducer opened 1 year ago

inducer commented 1 year ago

Benefits:

Drawbacks:

Naming of the flag is an open question.

cc @majosm @mtcam

kaushikcfd commented 1 year ago

I don't think I understand this. Could you please elaborate? Does this have to do with sub-expressions having the same value but different metadata? Thanks!

inducer commented 1 year ago

Sorry, I didn't spend enough time explaining. Right now, we have reasonably elaborate dispatch logic between different versions of a compiled function, based on shapes, which arguments are supplied, etc. This flag would assert that only a single version should ever exist, and calling the compiled function a second time with mismatched arguments would be an error. Does that make sense?

kaushikcfd commented 1 year ago

Yep, makes sense. Thanks! Essentially this flag should ensure that length of BaseLazilyCompilingFunctionCaller.program_cache should not exceed 1.

inducer commented 1 year ago

Yup, exactly.

majosm commented 1 year ago

Question: Would the allow_single_version=True case check if the arguments are the same as when it was compiled? Or is it more of an "at your own risk" thing?

kaushikcfd commented 1 year ago

Question: Would the allow_single_version=True case check if the arguments are the same as when it was compiled? Or is it more of an "at your own risk" thing?

It should be an error at this point if len(self.program_cache) > 1.

majosm commented 1 year ago

Question: Would the allow_single_version=True case check if the arguments are the same as when it was compiled? Or is it more of an "at your own risk" thing?

It should be an error at this point if len(self.program_cache) > 1.

Does it still incur the "dispatch cost to select which implementation to use" then?

kaushikcfd commented 1 year ago

Aah fair. I missed that as a constraint. Though I am unsure how to avoid the dispatch cost since we would always need to compute the arg_id_to_descr.

majosm commented 1 year ago

In any case, I can see the value in avoiding multiple compiles. On the user side, though, I'm not sure if it's always obvious enough when it's happening that one would know to intervene. Kind of makes me wonder if the single_version_only=True behavior should be the default? i.e., emit an error if it tries to compile multiple times, unless the caller explicitly says it's OK. Not sure how much stuff this would break, though.

inducer commented 1 year ago

At the very least, it would break the LSRK45 time stepper, which relies on having two different versions at the moment.

https://github.com/inducer/grudge/blob/c9906c9a20bfb5c78733093ccba716d7ac036897/grudge/shortcuts.py#L44-L62

matthiasdiener commented 1 year ago

How about just emitting a warning instead of an exception? This would even work without necessarily changing the API.

inducer commented 1 year ago

IMO, both multi-version and single-version are legitimate uses of the function, unless the user says otherwise.