Skip to content
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

add Context manager and default context under threading #38

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

add Context manager and default context under threading #38

wants to merge 12 commits into from

Conversation

thisiscam
Copy link
Contributor

@thisiscam thisiscam commented Oct 17, 2020

This implements #22

Semantics of context pushing is added as a test case.

With this change:

  • DEFAULT_CONTEXT is deprecated (with a warning) in favor of get_default_context()
  • get_default_context() always returns a thread unique default context for the first call in a thread.
  • push_context(ctx=none) can be used to push new default context(s), which allows for custom default Context.
  • Context now implements __copy__ and __deepcopy__ which always returns the identity.

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Some comments below.

islpy/__init__.py Show resolved Hide resolved
islpy/__init__.py Outdated Show resolved Hide resolved
islpy/__init__.py Show resolved Hide resolved
test/test_isl.py Show resolved Hide resolved
islpy/__init__.py Show resolved Hide resolved
islpy/__init__.py Show resolved Hide resolved
thisiscam and others added 3 commits October 18, 2020 20:21
Fix future warning date of DEFAULT_CONTEXT

Co-authored-by: Andreas Klöckner <inform@tiker.net>
@thisiscam
Copy link
Contributor Author

I have put TODO_VERSION everywhere in the docs. Please take a look and change them to something appropriate?

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few more minor things, then this is ready to go.

doc/ref_fundamental.rst Outdated Show resolved Hide resolved
.. note:: this class implements Python's ``__copy__`` and ``__deepcopy__``
protocals. Copying of objects of this class always returns the identity.

.. seealso:: :ref:`sec-context-management`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a note on pickle behavior.

Copy link
Contributor Author

@thisiscam thisiscam Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
However, I'm thinking towards a slightly better semantics of pickling of Context. I can't imagine a use case for this, but here's a sketch:

@functools.lru_cache(maxsize=None)
def _get_context_by_id(ctx_id):
        return Context()

def context_reduce(self):
        if self == get_default_context():
            return (get_default_context, ())
        return (_get_context_by_id, (hash(self), ))

What do you think?

doc/reference.rst Outdated Show resolved Hide resolved
islpy/__init__.py Outdated Show resolved Hide resolved
islpy/__init__.py Outdated Show resolved Hide resolved
return get_default_context()


import contextlib
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to top of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

islpy/__init__.py Show resolved Hide resolved
thisiscam and others added 7 commits October 19, 2020 13:04
Co-authored-by: Andreas Klöckner <inform@tiker.net>
Co-authored-by: Andreas Klöckner <inform@tiker.net>
Co-authored-by: Andreas Klöckner <inform@tiker.net>
Co-authored-by: Andreas Klöckner <inform@tiker.net>
@inducer
Copy link
Owner

inducer commented Oct 19, 2020

Update islpy/__init__.py

Please have proper commit messages.

@inducer
Copy link
Owner

inducer commented Oct 19, 2020

(OK to leave these, just for the future.)

@thisiscam
Copy link
Contributor Author

Sorry, I believe they were generated automatically by GitHub. I will definitely note this in the future.

@inducer
Copy link
Owner

inducer commented Oct 19, 2020

Sorry, I believe they were generated automatically by GitHub.

They are, but you have the option to change them when you click "Commit".

Base automatically changed from master to main March 8, 2021 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants