Context cannot be copied

[thisiscam]

Hi -

It seems like Context class has some oddities with copying.

import islpy
import copy

ctx = islpy.Context()
assert copy.copy(ctx).data == ctx.data                     # Fails
assert copy.copy(ctx).data == islpy.DEFAULT_CONTEXT.data   # Goes through

Is this the intended behavior?

related docs: https://documen.tician.de/islpy/reference.html#global-data

[inducer]

This new test based on your code codifies the expected behavior. I don't know how I could keep the contexts equality-comparable after pickling and unpickling (which copy amounts to). If you have an idea, I'm all ears.

[thisiscam]

I think it might make sense to have a context manager built into islpy, which is the most Pythonic way I can think of.
The API might look like this:

with islpy.new_context() as dctx:
   # islpy.DEFAULT_CONTEXT will be swapped to dctx inside the with block
   assert copy.copy(dctx).data == dctx.data

[inducer]

I'm not a fan of modifying global variables, but given that there isn't really a good way of getting a context into pickle other than this, I could see myself going along with this if you submit a PR, assuming the documentation for this functionality comes with appropriately big warning labels.

[thisiscam]

There are two ways that I see this:

1. change the API to make _get_default_context() a public and encourage the user to use that.
   This way we don't have to change the DEFAULT_CONTEXT behavior for now(we can deprecate it in favor of a more consistent API), while also implement context switching without touching the global variable.

So the usage of islpy will be this:

with islpy.new_context() as dcts:
  assert dcts == islpy.get_default_context()
  assert islpy.Set("{[0]}").get_ctx() == islpy.get_default_context()
  # deprecate DEFAULT_CONTEXT

2. use a technique like https://stackoverflow.com/questions/880530/can-modules-have-properties-the-same-way-that-objects-can so to raise islpy from a module into an object.
   This way DEFAULT_CONTEXT can be a computed property of that object, which also solves our problem about touching global variables.
   This technique is less intrusive to the current code base, but it depends on how much you think it is a hack.

[inducer]

For your point 1, I don't see the big difference. Ultimately, there's still some global state that's being modified, unless I'm mistaken?

Point 2: Sorry, not a fan.

[thisiscam]

Sorry I wasn't clear enough. I was thinking (in the back of mind) an implementation of islpy.get_default_context as following:

import threading
from threading import current_thread

_thread_local_storage = threading.local()

def get_default_context():
  default_ctx = getattr(_thread_local_storage, 'default_islpy_context', None)
  if default_ctx is None:
    _thread_local_storage.default_islpy_context = islpy.Context()
  return default_ctx  

This should IMO avoid possible issues with using globals, as per ISL documentation:

All manipulations of integer sets and relations occur within the context of an isl_ctx. A given isl_ctx can only be used within a single thread. All arguments of a function are required to have been allocated within the same context. There are currently no functions available for moving an object from one isl_ctx to another isl_ctx. This means that there is currently no way of safely moving an object from one thread to another, unless the whole isl_ctx is moved.

And without point 2's hack, I don't see how a single global variable DEFAULT_CONTEXT suffices to implement this thread local logic?

[inducer]

OIC, that's legitimate. Perhaps the cleanest semi-backward-compatible solution I can think of is to make DEFAULT_CONTEXT a sentinel value that means "call get_default_context?

[thisiscam]

What do you mean by sentinel value ... "call get_default_context"? Could you give an example?

[inducer]

How about this: We use this temporarily to deprecate (i.e. warn about) uses of DEFAULT_CONTEXT, switch internal use over to get_default_context, and make sure that get_default_context in the main thread returns DEFAULT_CONTEXT?

[thisiscam]

After some trying I realized that there needs to be some design questions answered first before delivering an implementation.
What should be the behavior of copy, deepcopy and pickle for Context (or islpy objects in general)?
There are a couple of questions in detail:

1. should copy, deepcopy and pickle have the same behavior?

Pros:

• can reload object through pickle with a context manager
• can copy object from a different context (potentially from a different thread) to the current thread.

with islpy.Context() as dctx:
  assert copy.copy(s).get_ctx() == dctx   # `s` possibly created from another thread
  assert pickle.load(...some binary...).get_ctx() == dctx

Cons:

• unclear how to copy an object without changing it's context. One option is that copy behaves differently when not under a context manager. e.g.

assert s.get_ctx() != islpy.get_default_ctx()
assert copy.copy(s).get_ctx() == s.get_ctx()
with islpy.Context() as dctx:
  assert copy.copy(s).get_ctx() != s.get_ctx()  # behavior different under context manager!

But this doesn't seem ideal.

2. should copy and deepcopy mean strict copys without changing the object's context, while pickle always look at the current default context?

Pros:

• can reload object through pickle with a context manager
• simple semantics

Cons:

• not easy to transfer object from one context to another (one option is to pickle and unpickle, or islpy may have to provide a separate API to do it more efficiently)

Let me know if I missed anything?

[inducer]

Why is there a need to copy anything? islpy objects are "morally immutable".

[thisiscam]

Yes, but the copy might be issued by a larger enclosing object:

class A():
   set: islpy.Set
copy.copy(A())

One would normally want A().set.get_ctx() == copy(A()).set.get_ctx()

Note:
of course, one could implement custom __copy__ in A to avoid copying the set (that's what I did in my code before); however IMO this seems non-optimal: it seemed like a problem better handled by islpy (that copy loses context equality for the code base before introducing pybind, though this problem doesn't really exist now)

[inducer]

not easy to transfer object from one context to another (one option is to pickle and unpickle, or islpy may have to provide a separate API to do it more efficiently)

I don't even know what isl's preferred way of moving objects between context is. Do you?

FWIW, all the options seem broken/random/underspecified. I guess I hate option 2 the least. You?

[thisiscam]

This means that there is currently no way of safely moving an object from one thread to another, unless the whole isl_ctx is moved.

Per ISL doc, this seems impossible at the moment. Dumping to a string and back seems like the only way...

Yes, I agree that Option 2 seems the least broken. I'm taking a stab at this then.