BUG: Python wrapping needs std::string pass by value in some cases #4776

aylward · 2024-07-13T14:22:18Z

std::string * isn't wrapped in the SWIG wrapping used by ITK. As a result, the returned object will be unknown if std::string is passed by reference or returned by reference in certain cases.

This commit fixes this issue for SpatialObjectProperty member functions.

Here is the behavior before these changes:

Here is the behavior with these changes:

N-Dekker · 2024-07-15T08:59:52Z

Modules/Core/SpatialObjects/include/itkSpatialObjectProperty.h

-  SetName(const std::string & name)
+  SetName(std::string name)


This change might cause a performance penalty when the string is large. What about using C++17 std::string_view? string_view is designed to be passed by-value, and it supports both std::string and const char* arguments efficiently (avoiding dynamic memory allocation).

Agreed - but I considered the issue of having it throw a warning message in python (see the commit message examples) versus the chance that the "name" of an object would be a long string that could cause copy and/or memory issues, and I thought getting rid of the python message was more likely/prevalent/important. Perhaps this could be considered a necessary evil?

We could also start changing things to string_view throughout the code, but perhaps that is better as a separate pull request that did such a change on a large portion of ITK, rather than hiding/embedding it as a change one function at a time?

ITK does have const std::string & parameters in many other places. Are all of these problematic for SWIG?

@thewtex Does nonobind have the same problem with returning string references?

Great question!!!

@blowekamp - wanted flag this for you, since the message trail is getting "long".

nanobind may have improved behavior, but I do not know the case for const std::string &.

N-Dekker · 2024-07-15T09:02:12Z

Modules/Core/SpatialObjects/include/itkSpatialObjectProperty.h

@@ -91,18 +91,12 @@ class ITKSpatialObjects_EXPORT SpatialObjectProperty
  GetAlpha() const;

  void
-  SetName(const std::string & name)
+  SetName(std::string name)
  {
    m_Name = name;


If you do pass std::string by-value, please consider std::move here, to avoid extra memory reallocation:

m_Name = std::move(name);

Cool idea. Will do as an amend :)

blowekamp · 2024-07-15T13:02:10Z

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.

I am would be surprised if the "in" type is not being handled correctly.

aylward · 2024-07-15T14:08:17Z

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.

I am would be surprised if the "in" type is not being handled correctly.

Me too!! I haven't followed what logic/heuristic is causing this in swig. I tried a ton of options from the web, but the function continues to throw the error message when python wrapped - see the details given in the pull request - I created a small test to verify this odd SWIG behavior - I show how a very short bit of code can reproduce this error without the proposed change, and how the proposed change fixes it.

blowekamp · 2024-07-15T14:38:22Z

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.
I am would be surprised if the "in" type is not being handled correctly.

Me too!! I haven't followed what logic/heuristic is causing this in swig. I tried a ton of options from the web, but the function continues to throw the error message when python wrapped - see the details given in the pull request - I created a small test to verify this odd SWIG behavior - I show how a very short bit of code can reproduce this error without the proposed change, and how the proposed change fixes it.

I don't see an error message or a "thrown" object in the commit message or subject. I see <Swig Object of type X at A> but that is the default printer for a SWIG wrapped object.

aylward · 2024-07-15T14:47:12Z

I would expect "out" type map for const std::string & and std::string to be returning a native Python str object.
I am would be surprised if the "in" type is not being handled correctly.

Me too!! I haven't followed what logic/heuristic is causing this in swig. I tried a ton of options from the web, but the function continues to throw the error message when python wrapped - see the details given in the pull request - I created a small test to verify this odd SWIG behavior - I show how a very short bit of code can reproduce this error without the proposed change, and how the proposed change fixes it.

I don't see an error message or a "thrown" object in the commit message or subject. I see <Swig Object of type X at A> but that is the default printer for a SWIG wrapped object.

Exactly! - and that message appears on the console whenever the Set function is called. As a result, when making changes to a dictionary, that message is printed every time and the console gets spammed. Here is a change of no major impact that eliminates that message - yup, not a error message (my mistake for calling it that, but in my application, it makes the user think they are seeing an error, my users aren't developers - so, it is "evil" spam. I can redirect all output from wrapping/python, but then I miss messages that I want to appear on the console. So, this PR fixes unwanted behavior that is an unwanted side effect (a subjective error :) ).

blowekamp · 2024-07-15T15:16:11Z

OK. There is no "error" message, just extra console output.

Why do you think there is a memory leak as written in the commit message?

Regarding the printing of the Object to the console, when there is a return value not assigned to a variable then interactive consoles will print that value. If however the results are assigned to a variable then it will not be printed. Why are there loops of GetName where the results are being unused?

Forgive me for all the questions, I am trying to undertand the underlying issue and it there are wide spread wrapping issues that need to be further investigated.

aylward · 2024-07-15T15:40:11Z

OK. There is no "error" message, just extra console output.

Yes - just spam.

Why do you think there is a memory leak as written in the commit message?

Sorry -my mistake - I'm prepping another commit that deal with a memory leak, not this one - I confused the messages. It is only printing spam and returning <swig objects> that cannot be easily used.

Regarding the printing of the Object to the console, when there is a return value not assigned to a variable then interactive consoles will print that value. If however the results are assigned to a variable then it will not be printed.

Agreed. But this message is being printed when the Set function, which returns void, is called. I have no idea why. Yes, it is perhaps a bigger issue with SWIG.

Regarding the GetName() - it is problematic to return "<Swig object....>" instead of a str that contains the answer. As-is, we cannot call "GetName()" from python - we will end up with a SWIG object that has no easy way of being cast to a str that python can use. The GetName() function is useless from python.

Why are there loops of GetName where the results are being unused?

Sorry - what are you referring to? What loops in the code? Ah - you think in my application's code I'm calling GetName and it is causing the messages because I'm not using the return value - LOL. Nope, my loops are calling SetTagStringValue() (see cell 6 in the sample code) - which prints the spam message even though the Set function return void! So, I am calling a function that returns void, yet it is spamming the console.

Forgive me for all the questions, I am trying to undertand the underlying issue and it there are wide spread wrapping issues that need to be further investigated.

I suspect that there are wider wrapping issues at play - but I do not have the cycles for trying to debug such things further.

I fully admit that with perfect wrapping, none of this should be happening - but I assume I'm hitting a corner case in ITK's wrapping. If y'all could figure it out, that would be outstanding! Perhaps, in the meantime, could we go with this. No backward compatibility is broken :)

dzenanz · 2024-07-15T16:17:52Z

/azp run ITK.macOS

std::string * doesn't have a desctructor in the SWIG wrapping used by ITK. As a result, spam messages are being written when std::string is passed by reference or returned by reference in certain cases. Also, return by references are remaining as abstract SWIG Objects that are not easy to convert to standard python types. This commit fixes this issue for SpatialObjectProperty member functions.

blowekamp · 2024-07-15T17:16:56Z

@aylward The commit message reads:

std::string * doesn't have a desctructor in the SWIG wrapping used
by ITK. As a result, memory leak warnings will be thrown if
std::string is passed by reference or returned by reference in
certain cases.

This commit fixes this issue for SpatialObjectProperty member
functions.

This sounded like a serious issue and got my attention.

I undertand return "std::string" for return values over pointers and references at this allows SWIG to map the results to a native python type.

After reproducing this example I did see the following message when exiting ipython:
swig/python detected a memory leak of type 'std::string *', no destructor found.

Clearly std::string * is bad.

It was initially not clear to me the issue with SetTagStringValue. This odd behavior is odd:

In [1]: import itk

In [2]: so = itk.TubeSpatialObject[3].New()
prop
In [3]: prop = so.GetProperty()

In [4]: prop.SetTagStringValue("Name", "foo")
Out[4]: <Swig Object of type 'std::string *' at 0x12c947600>

In [5]: r =  prop.SetTagStringValue("Name", "foo")

In [6]: print(r)
<Swig Object of type 'std::string *' at 0x105113270>

For some reason SWIG thinks SetTagStringValue is returning a std::string *.

SimpleITK consistently takes const std::string & as inputs and returns std::string as outputs and has not seen these issues.

aylward · 2024-07-15T17:17:57Z

Why do you think there is a memory leak as written in the commit message?

Sorry -my mistake - I'm prepping another commit that deal with a memory leak, not this one - I confused the messages. It is only printing spam and returning that cannot be easily used.

Fixed in an amended commit

thewtex

👍

I think the idea @N-Dekker brings for broader string_view is a good one, but a topic for a separate PR when used more broadly throughout the toolkit.

aylward · 2024-07-15T17:48:43Z

SWIG converts return by value to return by pointer:
https://www.swig.org/Doc1.3/SWIG.html#SWIG_nn23

so, our handling of string is really wonky and could produce a memory leak :(

I wonder if there is a rogue swig template specification, see this:
https://www.swig.org/Doc1.3/Arguments.html#Arguments_nn2

this would cause a function in C++ that returns a void to wrap to a python function that returns a value - hence causing the SetTagValueString to appear to return a value in python.

blowekamp · 2024-07-15T18:05:07Z

The docs you are linking to are 1.3, the current release is 4.2.1. I think the general points are still correct.

However, the documentation is about returning pointers is for general custom structures/objects. SWIG provides a std library of typemaps for C++ std library types such as std::string.

https://www.swig.org/Doc4.2/Library.html#Library_std_string

This typemap enables std::string to interface with a native (Python str) object in most cases were write access to the wrapped object is not needed.

I wonder if there is a rogue swig template specification, see this: https://www.swig.org/Doc1.3/Arguments.html#Arguments_nn2

this would cause a function in C++ that returns a void to wrap to a python function that returns a value - hence causing the SetTagValueString to appear to return a value in python.

Maybe.

N-Dekker · 2024-07-16T09:31:25Z

Modules/Core/SpatialObjects/src/itkSpatialObjectProperty.cxx

@@ -102,7 +102,7 @@ SpatialObjectProperty::SetTagScalarValue(const std::string & tag, double value)
 }

 void
-SpatialObjectProperty::SetTagStringValue(const std::string & tag, const std::string & value)
+SpatialObjectProperty::SetTagStringValue(const std::string & tag, std::string value)
 {
  m_StringDictionary[tag] = value;


Please std::move here as well 😺, when you pass the value "by value" . As in:

m_StringDictionary[tag] = std::move(value);

N-Dekker · 2024-07-16T09:37:49Z

Modules/Core/SpatialObjects/include/itkSpatialObjectProperty.h

-  SetTagStringValue(const std::string & tag, const std::string & value);
+  SetTagStringValue(const std::string & tag, std::string value);


Do I understand correctly that the SWIG problem occurred with the parameter value, but not with tag? If so, what would be the general guideline on when to pass std::string by-value, and when to pass it by const &?

I have no idea. :) We have only 1 data point at this time. SWIG seems to be somehow converting this function to a new version that returns a std::string * (rather than returning a void as defined by the C++ code). In another comment,, I reference documentation (albeit from an old version of SWIG, but perhaps still relevant) that suggests that SWIG supports wrapping templates that could be doing this conversion for Set functions that take multiple args. Furthermore, other documentation explains why returning std::string & might be converted to std::string * in the Get cases. I suspect there is a bug in our SWIG wrapping specification for these reasons and because new SWIG should handled std::string fine and these wonkynesses shouldn't be happening. I'll file an issue.

dzenanz · 2024-09-09T13:35:54Z

Should this be merged? Before it rots due to other changes?

blowekamp

I don't think this PR should be merged. More details about the correct solution are described here:
#4779 (comment)

This is an artifact of SWIG applying policies by name of argument not the by value vs reference here.

aylward · 2024-09-09T15:24:44Z

The method in #4779 is likely to break backward compatibility throughout the toolkit, right? I think these changes are the lesser evil. s

…

On Mon, Sep 9, 2024 at 9:40 AM Bradley Lowekamp ***@***.***> wrote: ***@***.**** requested changes on this pull request. I don't think this PR should be merged. More details about the correct solution are described here: #4779 (comment) <#4779 (comment)> This is an artifact of SWIG applying policies by name of argument not the by value vs reference here. — Reply to this email directly, view it on GitHub <#4776 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEJL6PCY6A25JOJH6PEJTZVWQN5AVCNFSM6AAAAABK2KDN32VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBZHE3DAMRZHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

blowekamp · 2024-09-09T17:17:11Z

The method in #4779 is likely to break backward compatibility throughout the toolkit, right? I think these changes are the lesser evil. s
…

I believe this is incorrect. The minimal change suggested is just changing the name of a parameter "value"->"in_out_value".

Although I am not sure I am back up to speed on everything. In trying to build on my Mac are not of the downloaded binaries work, and now trying on a linux box.

Also I am presuming the text in the opening is the only regression code to re-produce this issue?

blowekamp · 2024-09-11T13:17:45Z

superseded by #4843

github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances area:Core Issues affecting the Core module labels Jul 13, 2024

aylward requested review from dzenanz and N-Dekker July 13, 2024 14:24

N-Dekker reviewed Jul 15, 2024

View reviewed changes

aylward force-pushed the PassStringByValue branch from 21c0a39 to 945f766 Compare July 15, 2024 17:16

thewtex approved these changes Jul 15, 2024

View reviewed changes

N-Dekker reviewed Jul 16, 2024

View reviewed changes

aylward mentioned this pull request Jul 16, 2024

Wrapping of member functions that are passed and/or return std:string goes wonky #4779

Closed

This was referenced Jul 19, 2024

BUG: Memory leak in GetObjectSubType due to unused tag/record Kitware/MetaIO#131

Merged

MetaIO 2024-07-13 (918ad45f) #4778

Merged

blowekamp self-requested a review September 9, 2024 13:38

blowekamp requested changes Sep 9, 2024

View reviewed changes

blowekamp mentioned this pull request Sep 9, 2024

BUG: Remove SpatialObjectProperty's writable string methods from SWIG #4843

Merged

7 tasks

blowekamp closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Python wrapping needs std::string pass by value in some cases #4776

BUG: Python wrapping needs std::string pass by value in some cases #4776

aylward commented Jul 13, 2024 •

edited

Loading

N-Dekker Jul 15, 2024

aylward Jul 15, 2024 •

edited

Loading

N-Dekker Jul 15, 2024

dzenanz Jul 15, 2024

aylward Jul 15, 2024

thewtex Jul 15, 2024

N-Dekker Jul 15, 2024

aylward Jul 15, 2024 •

edited

Loading

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024 •

edited

Loading

dzenanz commented Jul 15, 2024

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024

thewtex left a comment

aylward commented Jul 15, 2024 •

edited

Loading

blowekamp commented Jul 15, 2024

N-Dekker Jul 16, 2024

N-Dekker Jul 16, 2024

aylward Jul 16, 2024

dzenanz commented Sep 9, 2024

blowekamp left a comment

aylward commented Sep 9, 2024 via email

blowekamp commented Sep 9, 2024

blowekamp commented Sep 11, 2024

		SetTagStringValue(const std::string & tag, const std::string & value);
		SetTagStringValue(const std::string & tag, std::string value);

BUG: Python wrapping needs std::string pass by value in some cases #4776

BUG: Python wrapping needs std::string pass by value in some cases #4776

Conversation

aylward commented Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

aylward Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylward Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024 • edited Loading

dzenanz commented Jul 15, 2024

blowekamp commented Jul 15, 2024

aylward commented Jul 15, 2024

thewtex left a comment

Choose a reason for hiding this comment

aylward commented Jul 15, 2024 • edited Loading

blowekamp commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzenanz commented Sep 9, 2024

blowekamp left a comment

Choose a reason for hiding this comment

aylward commented Sep 9, 2024 via email

blowekamp commented Sep 9, 2024

blowekamp commented Sep 11, 2024

aylward commented Jul 13, 2024 •

edited

Loading

aylward Jul 15, 2024 •

edited

Loading

aylward Jul 15, 2024 •

edited

Loading

aylward commented Jul 15, 2024 •

edited

Loading

aylward commented Jul 15, 2024 •

edited

Loading