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

[FIX] VizRankDialog: Use extended thread pool to prevent segfaults #3669

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Mar 12, 2019

Issue

When fed large datasets to owcorrelations, the widget exited with segmentation fault, (probably) due to insufficient stack size for created task.

To reproduce: Generate a dataset with at least 26 features and 3361 rows, and feed it to OWCorrelations.

Description of changes
  • VizRankDialog now inherits ConcurrentMixin, which uses ThreadExecutor that provides sufficient stack size.
Includes
  • Code changes
  • Tests
  • Documentation

@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #3669 into master will increase coverage by 0.05%.
The diff coverage is 98.86%.

@@            Coverage Diff             @@
##           master    #3669      +/-   ##
==========================================
+ Coverage   84.43%   84.48%   +0.05%     
==========================================
  Files         372      373       +1     
  Lines       68087    68188     +101     
==========================================
+ Hits        57488    57609     +121     
+ Misses      10599    10579      -20

@VesnaT VesnaT force-pushed the vizrank_threads branch 2 times, most recently from de3c95c to 80a1748 Compare March 13, 2019 10:34
@ales-erjavec
Copy link
Contributor

ales-erjavec commented Mar 15, 2019

I get various

-------------------------- AssertionError Exception ---------------------------
Traceback (most recent call last):
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/utils/concurrent.py", line 954, in _on_task_done
    self.on_done(future.result())
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/visualize/utils/__init__.py", line 327, in on_done
    assert not self.state_count() or self.rank_model.rowCount() == self.state_count()
AssertionError

or

-------------------------- AssertionError Exception ---------------------------
Traceback (most recent call last):
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/utils/concurrent.py", line 954, in _on_task_done
    self.on_done(future.result())
  File "/Users/aleserjavec/workspace/orange3/Orange/widgets/visualize/utils/__init__.py", line 327, in on_done
    assert self.saved_state is None
AssertionError
-------------------------------------------------------------------------------

if I Pause/Continue multiple time during a run then let it run till the end in Correlations widget (testing on phoneme.tab).

@VesnaT
Copy link
Contributor Author

VesnaT commented Mar 15, 2019

I just realized these asserts should be removed because an exception can occur while computing the score and therefore is not put into the table.
But while testing phoneme.tab that is not the case, since all the scores can be computed. So apparently there is a bug related to pausing calculation.

When fed large datasets, correlations widget exited with segmentation fault, (probably) due to insufficient stack size for created task.
@ales-erjavec
Copy link
Contributor

ales-erjavec commented Mar 15, 2019

Might I suggest just setting a larger initial stack size on the QThread instance, then maybe revisit this latter.

I think the ConcurrentMixin is not a good fit for this, at least as is.

@ales-erjavec ales-erjavec merged commit 9e0d4eb into biolab:master Mar 18, 2019
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