Skip to content
This repository has been archived by the owner on Nov 21, 2020. It is now read-only.

Remember previous context #31

Merged
merged 2 commits into from
Feb 26, 2020
Merged

Remember previous context #31

merged 2 commits into from
Feb 26, 2020

Conversation

Huy-Ngo
Copy link
Collaborator

@Huy-Ngo Huy-Ngo commented Feb 15, 2020

I try to resolve #30 by saving context into a static stack-list current_context (maybe I should rename it to current_contexts) in which the top (last) item is the current context. When a context is destroyed (__exit__), it's popped and the latest previous context will be loaded into use_context.

@McSinyx
Copy link
Owner

McSinyx commented Feb 18, 2020

As part of this PR, could you please also expose alure::Context::{Make,Get}ThreadCurrent, as part of use_context and (for-you-to-be-defined) current_context through a thread parameter or something (be creative on the name).

@McSinyx
Copy link
Owner

McSinyx commented Feb 21, 2020

Please also see comments on #30.

@McSinyx McSinyx mentioned this pull request Feb 21, 2020
@McSinyx McSinyx linked an issue Feb 21, 2020 that may be closed by this pull request
tests/test_context.py Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
Copy link
Owner

@McSinyx McSinyx left a comment

Choose a reason for hiding this comment

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

Mostly LGTM now, just a few adjustments in tests and API needed.

src/palace.pyx Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
tests/test_context.py Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
@McSinyx
Copy link
Owner

McSinyx commented Feb 24, 2020

I've just take a look at Travis build log and turns out it failed because alure::Context::GetCurrent is not exposed (in src/alure.pxd). BTW when you're done with this, please rebase all commits into two:

  • The test
  • The fix

in any order you find suitable. There are bunch of tutorials for interactive git rebase online.

src/palace.pyx Outdated Show resolved Hide resolved
@Huy-Ngo
Copy link
Collaborator Author

Huy-Ngo commented Feb 24, 2020

There is this error I don't know why happens:

================================================================================= ERRORS =================================================================================
__________________________________________________________________ ERROR at teardown of test_gain_auto ___________________________________________________________________

    @fixture(scope='session')
    def device():
        """Provide the default device."""
>       with Device() as dev: yield dev

tests/conftest.py:32: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/palace.pyx:175: in palace.Device.__exit__
    self.close()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   self.impl.close()
E   RuntimeError: Trying to close device with contexts

src/palace.pyx:329: RuntimeError

I suspect this has something to do with my edit of __enter__ and __exit__ of Context class. (Edit: I think it has something to do with get_current() not returning None: as I mentioned here, I am not sure if it will return a null pointer or not)

Also I don't understand why these 11 non-context tests failed even after I pulling from master. (Edit: nvm it failed on master too, this merge failed due to the error mentioned above).

@McSinyx
Copy link
Owner

McSinyx commented Feb 24, 2020

I think it's pretty clear why:

RuntimeError: Trying to close device with contexts

It's likely that there're contexts left undestroyed (I'm looking at the current_context function where Context(dev) is called then impl is replaced by another, but the older one isn't killed properly).

Also I don't understand why these 11 non-context tests failed

pytest with fixtures is very fragile when the fixtures get fucked, and since all tests uses the device fixture, it's normal that they failed.

even after I pulling from master.

I hope you meant after checking out master, then try to delete the compiled bytecodes in tests. There should be a way to disable these annoying caches, but I couldn't find that yet.

@Huy-Ngo
Copy link
Collaborator Author

Huy-Ngo commented Feb 24, 2020

Just looked through the builds above and it started failing here.

tests/test_context.py Outdated Show resolved Hide resolved
@McSinyx
Copy link
Owner

McSinyx commented Feb 24, 2020

Just looked through the builds above and it started failing here.

I don't think it's relevant, since we've changed the approach. Please try the suggested changes.

@Huy-Ngo
Copy link
Collaborator Author

Huy-Ngo commented Feb 25, 2020

Yay it passes the test. Is there any other change request before rebasing?

Copy link
Owner

@McSinyx McSinyx left a comment

Choose a reason for hiding this comment

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

Generally this looks good to me, aside a few tiny details.

Note on documentation, the explanation of context manager needs update after the change (that's why current_context is required in the first place, there's no way to describe what the with thing does without the function).

Just in case the comment requesting rebase get lost, split the patch into two commits: one for the change and one for the test, in any order. Cheers.

src/palace.pyx Outdated Show resolved Hide resolved
src/palace.pyx Outdated Show resolved Hide resolved
tests/test_context.py Outdated Show resolved Hide resolved
@Huy-Ngo
Copy link
Collaborator Author

Huy-Ngo commented Feb 25, 2020

Hmm... I wish I always made different commits for separate files. Is that okay if I leave some commit changing tests/test_context.py to be in the same one as src/palace.pyx because splitting it up is troublesome?

Aight, I think I need some practice first.

src/palace.pyx Outdated Show resolved Hide resolved
if alure.Context.get_current() == <alure.Context> nullptr:
return None
cdef Context current = Context.__new__(Context)
current.impl = alure.Context.get_current()
Copy link
Owner

@McSinyx McSinyx Feb 25, 2020

Choose a reason for hiding this comment

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

There's no point of calling alure.Context.get_current() twice. As defined in alure2.h, bool of alure::Context is the nonnullity, thus all you have to do is just if not current: return None. Also in the line below use __new__ to create the empty device, just for consistency (ugh why isn't @9a24f0 done with #38?). Remember to fixup the changes (git rebase -i HEAD~2).

After this, use the Rebase and merge button on Github to merge the changes. Then on branch master pump the version (in setup.cfg, to 0.0.10 I think, you can do it as part of the fixup) draft a new pre-release with sdist (./setup.py sdist) attached, wait for the wheels to get pushed to PyPI by Travis, then install it using pip. Switch to gh-pages branch, update the version (in src/conf.py), generate the new documentation and push. That's all the work for a new release lmao.

And don't forget to document how the context returned by current_context have the wrong message handler in a new issue (pseudo test: with context: assert current_context().message_handler is context.message_handler).

Copy link
Owner

Choose a reason for hiding this comment

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

Test passed, feel free to Rebase and merge when you're ready.

@Huy-Ngo Huy-Ngo force-pushed the context-mng branch 2 times, most recently from 55242c0 to 469a201 Compare February 26, 2020 02:34
src/palace.pyx Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing context Context managers managing Context does not manage contexts contextually
2 participants