Fix python tests on Windows #2530
Conversation
| @@ -59,7 +59,7 @@ vw_ptr my_initialize(std::string args) | |||
| { if (args.find_first_of("--no_stdin") == std::string::npos) | |||
| args += " --no_stdin"; | |||
| vw*foo = VW::initialize(args); | |||
| return boost::shared_ptr<vw>(foo, dont_delete_me); | |||
There was a problem hiding this comment.
should we leave a comment in case things go downhill in the future?
There was a problem hiding this comment.
particularly bc we're not sure of the impact and we also had some memory leak reported so there's something quirky going on with the python bindings. maybe a comment would be helpful as a breadcrumb in a future investigation
There was a problem hiding this comment.
I think leaving the rationale here would be ok. The code makes sense, and if a problem comes up, the commit history will eventually lead back to this PR. Sound reasonable?
|
Commenting on the change to the vw shared_ptr deleter function: This line incorrectly comments that python will delete the object; in actuality the object would be leaked because the default shared pointer deleter was replaced with the no-op deleter. |
* Fix python tests on Windows * move import to top Co-authored-by: Jack Gerrits <jackgerrits@users.noreply.github.com>
* Fix python tests on Windows * move import to top Co-authored-by: Jack Gerrits <jackgerrits@users.noreply.github.com>
Fixes a few things.
finish_examplecannot be called on an example that has not been used. Doing so on Windows will cause it to crash (this behavior appears to have existed for a while, an incorrect test simply exposed it)int64s. However, the default conversion ofint->pandas_dtypeon Windows isint->int32. This causes type mismatch errors when performing our type checks in DFtoVW.I'm not completely sure of the ramifications of the vw deletion change though; on the surface it makes sense and all of the tests pass. However there may be some subtle interactions with how we implement the python wrappers or how they're being used that could cause issues.