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

HPCC-16906 Add a new plugin for embedded Python3 code #9644

Merged
merged 1 commit into from Mar 3, 2017

Conversation

Projects
None yet
5 participants
@richardkchapman
Copy link
Member

commented Mar 1, 2017

Signed-off-by: Richard Chapman rchapman@hpccsystems.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browers
    • The component(s) render as expected

Testing:

Cloned the python2 regression suite tests, and ran them

@hpcc-jirabot

This comment has been minimized.

Copy link

commented Mar 1, 2017

https://track.hpccsystems.com/browse/HPCC-16906
Jira not updated (user does not match)

@richardkchapman richardkchapman force-pushed the richardkchapman:python3 branch from 5ce3623 to b1123e3 Mar 1, 2017

@AttilaVamos

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

@richardkchapman It was Python3 environment problem, fixed. This PR rescheduled.

if(${CMAKE_VERSION} VERSION_LESS "2.8.9")
message(WARNING "Cannot set NO_SONAME. shlibdeps will give warnings when package is installed")
elseif(NOT APPLE)
set_target_properties(pyembed PROPERTIES NO_SONAME 1)

This comment has been minimized.

Copy link
@AttilaVamos

AttilaVamos Mar 2, 2017

Contributor

I think this should be

set_target_properties(py3embed PROPERTIES NO_SONAME 1)

This comment has been minimized.

Copy link
@richardkchapman

richardkchapman Mar 2, 2017

Author Member

I think you are right...

This comment has been minimized.

Copy link
@richardkchapman

richardkchapman Mar 2, 2017

Author Member

repushed to fix

@richardkchapman richardkchapman force-pushed the richardkchapman:python3 branch from b1123e3 to b800fbd Mar 2, 2017

@AttilaVamos

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

@richardkchapman It seems this result is much better. I will tune a bit the log processing and GitHub comment generation and reschedule it.

@richardkchapman

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

@ghalliday Please review.

There's a lot of code in common with the Python2 plugin, and I slightly wonder if I should common them up - but then again I don't tend to like having one .cpp file compiled more than once (and they can't share the obj files - the python headers they include are different...

@ghalliday
Copy link
Member

left a comment

@richardkchapman a few minor comments/discussion items.

@@ -634,7 +634,7 @@ bool SafePluginMap::addPlugin(const char *path, const char *dllname)
if (!dll)
{
Owned<PluginDll> n = new PluginDll(path, NULL);
if (!n->load(true, false) || !n->init(pluginCtx))
if (!n->load(false, false) || !n->init(pluginCtx))

This comment has been minimized.

Copy link
@ghalliday

ghalliday Mar 3, 2017

Member

Are there any implications of this change? I think it is probably correct - but it might be worth commenting the logic.

This comment has been minimized.

Copy link
@richardkchapman

richardkchapman Mar 3, 2017

Author Member

There would be potential implications if we had multiple plugins that shared a common library that was not linked in by the system - might end up with two copies of it - but I'm not aware that we do so anywhere. Regression suite all seems fine.

I can put in a comment explaining the decision.

#define Py_TPFLAGS_HAVE_ITER 0
#endif

#define ASCII_LIKE_CODEPAGE "iso-8859-1"

This comment has been minimized.

Copy link
@ghalliday

ghalliday Mar 3, 2017

Member

Should come from #include of "jmisc.hpp"

This comment has been minimized.

Copy link
@richardkchapman

richardkchapman Mar 3, 2017

Author Member

I will change

PyErr_Clear();
assertex(PyUnicode_Check(valStr));
OwnedPyObject temp_bytes = PyUnicode_AsEncodedString(valStr, ASCII_LIKE_CODEPAGE, "ignore");
failx("%s", PyBytes_AsString(temp_bytes));

This comment has been minimized.

Copy link
@ghalliday

ghalliday Mar 3, 2017

Member

a discussion point. It is debatable whether this should be converting to ascii. I suspect we should be using utf8 for all our messages and enforcing all user output into that format. Utf8 should be the default unless otherwise specified by the user.

This comment has been minimized.

Copy link
@richardkchapman

richardkchapman Mar 3, 2017

Author Member

Yes, I wasn't sure whether ascii or utf8 was more appropriate here though in practice I doubt it makes a lot of difference. I'll change to utf8 if you prefer?

Changes made

@richardkchapman

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@ghalliday Please re-review/merge

@richardkchapman richardkchapman force-pushed the richardkchapman:python3 branch from d976b2a to 2602083 Mar 3, 2017

@ghalliday
Copy link
Member

left a comment

@richardkchapman
The following examples in the LN regression suite fail:

bug49457.eclxml
rnaren2.eclxml

Probably because a tree is transformed inconsistently.

Comment added in error

@ghalliday
Copy link
Member

left a comment

@richardkchapman one comment

unset(PYTHON_INCLUDE_DIR CACHE)
unset(PYTHON_DEBUG_LIBRARIES CACHE)
unset(PYTHONLIBS_VERSION_STRING CACHE)
ADD_PLUGIN(py3embed PACKAGES PythonLibs MINVERSION 3.2)

This comment has been minimized.

Copy link
@ghalliday

ghalliday Mar 3, 2017

Member

PyUnicode_AsUTF8AndSize() is only added in 3.3, so need to increase this version.

@richardkchapman richardkchapman force-pushed the richardkchapman:python3 branch from 2602083 to 427b902 Mar 3, 2017

Repushed to fix latest comment

@richardkchapman

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@ghalliday repushed. Let me know when you are done reviewing and I can squash

@ghalliday

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

@richardkchapman please can you squash

HPCC-16906 Add a new plugin for embedded Python3 code
Copied from the Python2 plugin, with modifications to reflect the changed C
API.

Note that libpython3.x exports many of the same symbols as libpython2.x, whcih
can lead to issues if both are loaded. In particular, I had to change the
visibility rules when loading plugins, and I also had to refine the code for
working around Centos distro issue with improperly linked Python extensions.
We now only apply the workaround if py3embed is not in use - people using
centos and buggy python distros will need to pick one or the other version of
pyembed, but not both.

Also discovered that one of the existing tests was working by luck (since
python sets are unordered), and stopped working on (at least some) python 3
installations.

Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

@richardkchapman richardkchapman force-pushed the richardkchapman:python3 branch from 427b902 to 5258c33 Mar 3, 2017

@richardkchapman

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@ghalliday Squashed

@ghalliday ghalliday merged commit 2fb47c8 into hpcc-systems:candidate-6.4.0 Mar 3, 2017

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

Automated Smoketest:
Sha: 427b902
Build: success
ECL Watch: Rebuilding Site

errors warnings build time
0 65 91.371 seconds

Install hpccsystems-platform-community_6.3.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout
unittest 93 93 0 0 0
wutoolTest(Dali) 19 19 0 0 0
wutoolTest(Cassandra) 19 19 0 0 0

Regression test result:

setup hthor thor roxie
roxie_echo Pass Pass Pass
roxie_keepwhitespace Pass Pass Pass
setup Pass Pass Pass
setup_fetch Pass Pass Pass
setupdict Pass Pass Pass
setupsearchindex Pass Pass Pass
setupsq Pass Pass Pass
setupwordindex Pass Pass Pass
setupxml Pass Pass Pass
test hthor thor roxie
embedpy3 Pass Excl. Pass
embedpy3-catch Pass Excl. Pass
embedpy3-fold Pass Excl. Pass
embedpy3a Pass Pass Pass
py3import Pass Pass Pass
py3streame Pass Pass Pass
py3streame2 Pass Pass Pass
py3streame3 Pass Pass Pass
streame Pass Pass Pass

HPCC Stop: OK
HPCC Uninstall: OK

@richardkchapman richardkchapman deleted the richardkchapman:python3 branch Jul 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.