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

Remove plot api locking. #2218

Merged
merged 1 commit into from
Oct 31, 2016
Merged

Conversation

bjlittle
Copy link
Member

@bjlittle bjlittle commented Oct 26, 2016

This PR addresses the post-merge issues raised by @dkillick on #2206.

Locking at the iris plot level will not guarantee to the user thread-safe plotting. In certain circumstances there is still the possibility of an iris plot using an mpl figure from a different thread. In the case of #2206, the implementation introduced a locking mechanism at the iris plotting api level, which is in fact at too low a level.

If a user insists on using iris plotting in a threaded context, then we should advocate that they take responsibility for locking themselves (which, to be fair, is pretty simple to do) in combination with creating the target mpl figure/axes (within the safety of the locked critical region).

Note that, for clarity, we do require to maintain non re-entrant locking at the individual graphics test case level in order to avoid the issues raised in #2195.

Closes #2210.

@bjlittle bjlittle added this to the v1.11 milestone Oct 26, 2016
@bjlittle
Copy link
Member Author

@dkillick Ping ...

@DPeterK
Copy link
Member

DPeterK commented Oct 27, 2016

@bjlittle spotted! Now all I need is some time to look through this...

@bjlittle
Copy link
Member Author

@dkillick Cool, thanks. Mind to review my responses to your comments on #2206 to make sure you're in agreement and happy with the closure of #2210

@DPeterK
Copy link
Member

DPeterK commented Oct 31, 2016

One thing this PR does not address is removing the pip install from travis.yml. Depending on the scope of this PR, I'm either happy with this or not happy with this:

  • if the scope of this is only remove spurious plotting locks, them that's fine (but can we get this done in a followup PR - see this comment), or
  • if the scope of this is backing out all that's wrong with Perceptual image hashing #2206 then I think it should be done here.

Comments on a postcard 📮! Or, you know, on this PR.

@DPeterK
Copy link
Member

DPeterK commented Oct 31, 2016

Otherwise I'm happy that the changes here represent a complete removal of the additions to plot.py that were introduced in #2206 👍

@bjlittle
Copy link
Member Author

@dkillick I've not address removing conda install pip from the .travis.yml file as this has already been done by #2211 on v1.11.x branch.

This PR is only to address removing the locking from the plotting api.

I don't quite understand your comment regarding backing out all that's wrong with #2206, care to elaborate ... what else is wrong?

@DPeterK
Copy link
Member

DPeterK commented Oct 31, 2016

I don't quite understand your comment regarding "backing out all that's wrong with #2206"

Sorry! What I meant was "do we want this PR to deal with all problems introduced in #2206", which to my mind includes the spurious pip install addition in .travis.yml. I think my question is answered by this comment...

This PR is only to address removing the locking from the plotting api

and as such I think we might be there with this fix! Thanks @bjlittle

@DPeterK DPeterK merged commit e87ab88 into SciTools:v1.11.x Oct 31, 2016
@bjlittle bjlittle deleted the remove-plot-lock branch November 12, 2019 14:11
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.

2 participants