Skip to content

BF: lock across threads check/instantiation of Flyweight instances - #7075

Merged
yarikoptic merged 3 commits into
datalad:maintfrom
yarikoptic:bf-parallel-instances
Oct 8, 2022
Merged

yarikoptic merged 3 commits into
datalad:maintfrom
yarikoptic:bf-parallel-instances

Conversation

@yarikoptic

Copy link
Copy Markdown
Member

As commented added to the comment says we might want (eventually) make locking smarter and per "id" since in principle we should be able to instantiate multiple separate instances in parallel. Might be useful or even critical for parallel "create" and alike. But I think such use-cases are rare so decided to KISS. test added, so disable locking to see how things could go wrong (hint: there is more than 1 way ;) ).

Fixes #6598

@yarikoptic yarikoptic added semver-patch Increment the patch version when merged CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Oct 7, 2022
@github-actions github-actions Bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Oct 7, 2022
@qlty-cloud-legacy

Copy link
Copy Markdown

Code Climate has analyzed commit 8eaffc4 and detected 0 issues on this pull request.

View more on Code Climate.

yarikoptic and others added 2 commits October 7, 2022 17:50
As commented added to the comment says we might want (eventually) make
locking smarter and per "id" since in principle we should be able to
instantiate multiple separate instances in parallel.  Might be useful or
even critical for parallel "create" and alike.  But I think such use-cases
are rare so decided to KISS.  test added, so disable locking to see how things
could go wrong (hint: there is more than 1 way ;) ).

Fixes datalad#6598
@yarikoptic
yarikoptic changed the base branch from master to maint October 7, 2022 21:51
@yarikoptic
yarikoptic force-pushed the bf-parallel-instances branch from 8eaffc4 to eed9496 Compare October 7, 2022 21:52
@codecov

codecov Bot commented Oct 7, 2022

Copy link
Copy Markdown

Codecov Report

Base: 74.92% // Head: 74.92% // No change to project coverage 👍

Coverage data is based on head (dc90cc9) compared to base (dc90cc9).
Patch has no changes to coverable lines.

❗ Current head dc90cc9 differs from pull request most recent head 4fb3bfd. Consider uploading reports for the commit 4fb3bfd to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            maint    #7075   +/-   ##
=======================================
  Coverage   74.92%   74.92%           
=======================================
  Files         354      354           
  Lines       58945    58945           
  Branches     6310     6310           
=======================================
  Hits        44164    44164           
  Misses      14767    14767           
  Partials       14       14           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@bpoldrack bpoldrack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thx!

While I am not entirely sure that this was the issue #6598, it seems possible and this locking should be an improvement either way.

Comment thread datalad/support/tests/test_parallel.py Outdated
_singleton though would have better represented the purpose/mechanics of the test -- to ensure a single instance of each GitRepo in the batch.

Co-authored-by: Benjamin Poldrack <bpoldrack@users.noreply.github.com>
@yarikoptic
yarikoptic merged commit 9af89be into datalad:maint Oct 8, 2022
@yarikoptic

Copy link
Copy Markdown
Member Author

While I am not entirely sure that this was the issue #6598, it seems possible and this locking should be an improvement either way.

I would leave 10% case that indeed it was something else since I do not remember if that test should have somehow instantiated the same dataset at different time points. But from the signature of the failure, and the fact that we get exactly that error of introduced test if you remove locking -- I am almost certain it is the same issue, so I will close it there.

@yarikoptic-gitmate

Copy link
Copy Markdown
Collaborator

PR released in 0.17.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible race condition in datalad.support.tests.test_parallel.test_creatsubdatasets

3 participants