Attributes of fields defined at the class level should not be shared between instances

[ylep]

I am having trouble with the Capsul tests for highres-cortex:

• when I test each process or pipeline separately all works as expected

  python -m highres_cortex.test.test_capsul SphereTestCase.test_laplacian
  python -m highres_cortex.test.test_capsul SphereTestCase.test_equivolumetric_pipeline
  ...
  

• but, whenever I run the whole test suite, I get random failures

  python -m highres_cortex.test.test_capsul SphereTestCase
  

The problem is that when I call highres_cortex.capsul.processes.Laplacian after is has been run as part of a pipeline, its fields retain the generate_temporary attribute!

I am using class-level attributes and not instance attributes, because I thought that it was more in line with the philosophy behind traits / pydantic typing.

[denisri]

I'm not sure if it is normal or not (I'd say not...), however in tests we generally try to use temporary/local databases. For instance in capsul.test.test_fake_morphologist, we use:

    def setUp(self):
        self.tmp = tmp = Path(tempfile.mkdtemp(prefix="capsul_test_fakem"))
        # [...]
        self.capsul = Capsul(
            "test_fake_morphologist",
            site_file=self.config_file,
            user_file=None,
            database_path=osp.join(self.tmp, "capsul_engine_database.rdb"),
        )

The reason for this was primarily not to foul the persistent user database, but it also avoids concurrent accesses.
Would it improve anything for you ?
Anyway your problem seems to point out that concurrent accesses are not safe...

[ylep]

Thanks @denisri for the tip! I have made the change to use a temporary local database as you suggested. Unfortunately, this did not fix the original issue.

I noticed that it is always the same two tests that are consistently failing (test_laplacian and test_upwind_euclidean). I will triple-check that the issue is not on my side...

By the way, since using a temporary database I am getting messages printed to my terminal asynchronously (server has probably shutdown. Exiting.). This is probably harmless, but is a bit annoying.

[ylep]

I made some progress and characterized the bug a bit more precisely: it happens when certain processes are run in a certain order. For instance, running highres_cortex.capsul.processes.Laplacian after highres_cortex.capsul.isovolume fails systematically:

KEEP_TEMPORARY=1 python -m highres_cortex.test.test_capsul SphereTestCase.test_equivolumetric_pipeline SphereTestCase.test_laplacian --verbose

It behaves as if the highres_cortex.capsul.processes.Laplacian process worked correctly the first time, but somehow got broken after being used in the pipeline (which makes use of it internally). Still investsigating...

[ylep]

Okay, here is the issue: even though I set the output filename (laplace_field) in the code that calls Laplacian:

        p = capsul.api.executable(
            "highres_cortex.capsul.processes.Laplacian")
        p.classif = os.path.join(self.test_dir, "classif.nii.gz")
        p.precision = 0.001
        p.typical_cortical_thickness = 3
        p.laplace_field = os.path.join(self.test_dir, "laplacian.nii.gz")   # <-- HERE
        print(p.laplace_field)
        with self.capsul_engine as ce:
            ce.run(p, print_report=True, debug=True)

it is replaced by Capsul with an auto-generated temporary filename:

job uuid: 5afd0e31-673d-49bb-ba10-35cf13303096
process: highres_cortex.capsul.processes.Laplacian
pipeline node: 
status: done
return code: 0
start time: 2023-11-14 11:54:22.979192
end time: 2023-11-14 11:54:23.938594
disabled: False
wait for: ['ab0aa528-d0a1-4e47-a02c-8dc3102de597']
waited_by: []
input parameters:
{'classif': '/tmp/highres-cortex-capsul-teststcf7p58_/classif.nii.gz',
 'laplace_field': '!{dataset.tmp.path}/Laplacian.laplace_field_2b39153a-4cec-44af-a58e-c39c952670d9.nii.gz',   <-- HERE
 'precision': 0.001,
 'typical_cortical_thickness': 3,
 'verbosity': 1}
output parameters: none

This happens if the pipeline (highres_cortex.capsul.isovolume) has been run first, not in a fresh client.

I will now inspect the Process class to see if it differs between the working and the non-working case.

[denisri]

I did not take time to look at your pipelines yet, but this definitely looks like a bug in Capsul.
Is the parameter which has a problem linked to a sub-process which is instantiated in both pipelines ?
(I wonder if there is a kind of class-level field which would erroneously share a value between instances, or something similar)

[ylep]

Found it! And yes @denisri you guessed it right: the problem is that when I call highres_cortex.capsul.processes.Laplacian after is has been used in the pipeline, its fields retain the generate_temporary attribute!

I am using class-level attributes and not instance attributes, because I thought that it was more in line with the philosophy behind traits / pydantic typing.

https://github.com/neurospin/highres-cortex/blob/b9b76f008c4b98a828a4267861498c790b13492b/python/highres_cortex/capsul/processes.py#L51-L71

Isn't there a way that we could store instance-level attributes? Maybe the typing fields are not the right place to store such attributes...

[denisri]

OK, then at least we understand.
Actually we use class-level fields as if they were instance fields, just as a more convenient way to declare them, but currently it's wrong. In Capsul v2, class traits were in fine duplicates as instance traits, so we did not run into this problem (class-level traits thus were not possible). In Capsul v3 we have probably not taken care of this, and fields are not duplicated in instances, thus do not behave the same.
So we have to decide which behaviour we want to implement (assuming it's possible to automatically duplicate class fields on instances).
Do we want to actually have class-level fields (more "pythoninc"), or always have instance fields (as in Capsul v2) ?

Isn't there a way that we could store instance-level attributes? Maybe the typing fields are not the right place to store such attributes...

Maybe not, it's just convenient and easy to do it this way. Another way would probably require some additional development.

[sapetnioc]

In v3, metadata are obtained from the Field instances and for class fields they are stored in class and therefore shared with all instances. It would not be easy to change the API to get metadata from controller instance since metadata are used in many places and can be read/modified by direct attribute access and there is no obvious method to find in code.

So, one option would be to copy all class fields into Process instances during construction. I have the feeling that it may use too much resource for big pipelines but I am not absolutely sure.

Another option could be to make class fields read-only (to avoid users to have the same kind of problem one day), to allow explicit copy of a class fields into an instance field to make it read-write and to store generate_temporary values in a dict in executable instead of in each field. I do not know if there are other internal metadata that are used that way but making class field read-only would raise an exception.

[denisri]

The copy to instance mechanism can be a solution, and could be done on demand, only for fields for which we want to change metadata. So yes why not. We would need an easy copy function, like Controller.make_instance_field(field_name) or something.