Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Apr 20, 2020

I realized that since #7612 has been merged, Circle reaches timeout after plot_visualize_stc. I can't reproduce locally so I decided to investigate in here.

  • Find that the issue is related to the wrapping of _Brain.save_movie
  • Resolve save_movie reference cycle (waiting for CircleCI)
  • Fix UnboundLocalError: playback_speed_slider used without being declared

@GuillaumeFavelier GuillaumeFavelier self-assigned this Apr 20, 2020
@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #7634 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #7634      +/-   ##
==========================================
- Coverage   85.60%   85.59%   -0.01%     
==========================================
  Files         454      454              
  Lines       83540    83539       -1     
  Branches    13218    13218              
==========================================
- Hits        71512    71506       -6     
- Misses       9314     9317       +3     
- Partials     2714     2716       +2     

@larsoner
Copy link
Member

@GuillaumeFavelier I think it's more likely a general memory problem. Sometimes it dies on plot_visualize_stc but also sometimes on plot_mne_solutions. Here is the mprof run plot_mne_solutions.py:

Screenshot from 2020-04-20 08-54-39

And plot_visualize_stc:

Screenshot from 2020-04-20 08-58-16

It's not the same as what happens on CircleCI, because Sphinx-gallery will close and GC at the end of each plotting block. And the SG report confirms this:

https://mne.tools/dev/auto_tutorials/source-modeling/sg_execution_times.html

Same on this PR, so maybe it does indeed fix it (?):

https://19456-1301584-gh.circle-artifacts.com/0/dev/auto_tutorials/source-modeling/sg_execution_times.html

In any case, this all indicates that the memory usage of _Brain can be high so we need to make sure it gets closed and GC'ed properly.

@GuillaumeFavelier
Copy link
Contributor Author

So if I understand correctly you think that the failures on CircleCI are not related to memory but we should pay attention to it? I can open a PR about that and make it my priority. Using those 2 scripts as benchmarks. What do you think?

@larsoner
Copy link
Member

So if I understand correctly you think that the failures on CircleCI are not related to memory but we should pay attention to it?

Either that or somehow they are related to memory and this PR fixes it (?). I have no idea why they would, though. copy_doc should work with str so it should make no reference cycles and having _Brain in the space doesn't seem like it should matter. Any idea why this PR seems to fix things?

Also should we merge it?

@GuillaumeFavelier
Copy link
Contributor Author

I am not sure myself. I wanted to investigate more actually:

  • restoring copy_doc
  • restoring the original function of _Brain during clean

@GuillaumeFavelier
Copy link
Contributor Author

Any way of getting more information really.

@GuillaumeFavelier
Copy link
Contributor Author

I will push a hotfix for the missing slider too.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Apr 20, 2020

The patch was not enough. I suggest to revert back to something similar to edb8ccb and merge with master.

I will find a way to put this feature back in another PR.

@GuillaumeFavelier GuillaumeFavelier marked this pull request as ready for review April 20, 2020 16:23
@GuillaumeFavelier GuillaumeFavelier changed the title TST: Check Circle timeout after plot_visualize_stc FIX: Circle timeout after plot_visualize_stc Apr 20, 2020
@GuillaumeFavelier
Copy link
Contributor Author

This is now ready to merge @larsoner

@larsoner
Copy link
Member

Thanks @GuillaumeFavelier !

@larsoner larsoner merged commit 7f8f5b5 into mne-tools:master Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants