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

Annotation class attrs #123

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

scottstanie
Copy link
Contributor

@scottstanie scottstanie commented Jun 28, 2023

This is trying to prevent #122 from popping up sporadically.
This is changing the classmethods in the annotation module to return instances: https://stackoverflow.com/a/14605349/4174466
All functions that are decorated with @classmethod are supposed to be alternate constructors (in addition to __init__); that is, they're meant to create a new instance. Right now, they have been changing "class attributes", which are shared across all instances of that class, and they are returning the class itself.

Why this is a problem
If we kick of many instance of COMPASS in different threads, each one that reads bursts instantiates these annotation classes. they are all changing the same class attribute, so it's a data race where difference function calls to .from_et can clobber each other.

(example to show what this looks like)

In [11]: from concurrent.futures import ThreadPoolExecutor
In [12]: import time, random

In [13]: class A:
    ...:     class_thing = 0

    ...:     @classmethod
    ...:     def from_x(cls, x):
    ...:         cls.class_thing = x

    ...:         # pretend we're doing some other parsing here in the construction
    ...:         time.sleep(random.random())

    ...:         # This is supposed to print the same thing twice:
    ...:         print(f"{x}, {cls.class_thing}")

    ...:         return cls()

In [14]: a1 = A.from_x(3)
3, 3

In [15]: a2 = A.from_x(2)
2, 2

if you try to do this method in many threads:

In [24]: tpe = ThreadPoolExecutor(max_workers=20)

In [26]: alist = list(tpe.map(A.from_x, range(20)))
3, 19
18, 19
16, 19
7, 19
2, 19
8, 19
14, 19
6, 19
9, 19
12, 19
0, 19
11, 19
19, 19
5, 19
15, 19
4, 19
1, 19
10, 19
17, 19
13, 19

Copy link
Contributor

@seongsujeong seongsujeong left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @scottstanie. I think the updates here are important to avoid the race condition when concurrent threads exists like the example you posted here.

Here are my comments and suggestions - Seongsu

src/s1reader/s1_annotation.py Show resolved Hide resolved
src/s1reader/s1_annotation.py Outdated Show resolved Hide resolved
src/s1reader/s1_annotation.py Outdated Show resolved Hide resolved
src/s1reader/s1_annotation.py Outdated Show resolved Hide resolved
src/s1reader/s1_annotation.py Show resolved Hide resolved
src/s1reader/s1_annotation.py Outdated Show resolved Hide resolved
Comment on lines 453 to 476
cls.number_of_samples = \
cls._parse_scalar('imageAnnotation/imageInformation/numberOfSamples',
number_of_samples = \
cls._parse_scalar(et_in,
'imageAnnotation/imageInformation/numberOfSamples',
'scalar_int')
cls.number_of_samples = \
cls._parse_scalar('imageAnnotation/imageInformation/numberOfSamples',
number_of_samples = \
cls._parse_scalar(et_in,
'imageAnnotation/imageInformation/numberOfSamples',
'scalar_int')
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's my bad again. It seems like that we have a duplicate here.

src/s1reader/s1_annotation.py Outdated Show resolved Hide resolved
src/s1reader/s1_annotation.py Show resolved Hide resolved
src/s1reader/s1_annotation.py Outdated Show resolved Hide resolved
Co-authored-by: Seongsu Jeong <sjeong.kr@gmail.com>
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