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-16754 Python embedded languages integrated into platform #10600

Merged
merged 1 commit into from Dec 19, 2017

Conversation

Projects
None yet
6 participants
@Michael-Gardner
Copy link
Contributor

commented Nov 3, 2017

Signed-off-by: Michael Gardner michael.gardner@lexisnexisrisk.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 improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • 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 browsers
    • The component(s) render as expected

Testing:

@hpcc-jirabot

This comment has been minimized.

Copy link

commented Nov 3, 2017

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

@xwang2713 This moves py2embed and py3embed into the platform. Please review. The plugins/py(x)embed/CMakeLists.txt files simply have the if(PYEMBED) condition removed, the rest is fixed indentation.

@johnholt I doubt I'll be able to make dependencies on packages installed by pip. That will most likely need to be a change in documentation. I can talk with @g-pan about whatever we decide to do.

This PR will most likely need to be modified once we merge #10587

@xwang2713

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

Do we need add python2 or python3 to cmake_modules/dependencies/? Actually the latest Ubuntu distro probably only has Python 3 installed by default. So we may need add python2 dependency

@xwang2713

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

I take a back. python2 will still available in future Ubuntu release

@xwang2713

This comment has been minimized.

Copy link
Member

commented Nov 3, 2017

Looks good

@Michael-Gardner Michael-Gardner force-pushed the Michael-Gardner:HPCC-16754 branch from 8a6eb29 to bb33f56 Dec 5, 2017

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

@richardkchapman I have rebased this against master and fixed the conflicts in the pyembed/py3embed CMakeLists.txt. It's ready to be merged if you want to take a look at it.

@richardkchapman

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

If I'm reading the diff correctly, this is basically removing the IF() tests around the building of the python plugins.

It would be better if the conditions were retained for use by developers that want to build on systems where python libraries are not installed, and we just adjusted when they are set (and potentially what they are called).

@Michael-Gardner

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

Ok I'll make those changes.

@Michael-Gardner Michael-Gardner force-pushed the Michael-Gardner:HPCC-16754 branch 2 times, most recently from 758430d to b6f736d Dec 11, 2017

HPCC-16754 Add py2embed and py3embed to platform package by default
Signed-off-by: Michael Gardner <michael.gardner@lexisnexis.com>

@Michael-Gardner Michael-Gardner force-pushed the Michael-Gardner:HPCC-16754 branch from b6f736d to 6d21457 Dec 11, 2017

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

@AttilaVamos What exactly happened with the smoketest here? Was there an issue with the smoketest as a whole, or is something actually wrong with my branch?

@AttilaVamos

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

There was some strange thing around. I think it would be better if I reschedule this PR and let us see what happen.

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

Thanks Attila!

@AttilaVamos

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

It is very likely your result won't be clean based on something wrong with unittests, but this isn't related to this PR.

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

@richardkchapman I tested it this way against LN and CE builds, and with Ubuntu 17.04 and Centos 7. Instead of the changes to the Plugin CMakeLists.txt files, I just enabled the INCLUDE_PYxEMBED variable by default for both 2 and 3.

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2017

Automated Smoketest:
Sha: 6d21457
Build: success
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

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

Regression test result:

phase total pass fail
setup (hthor) 11 11 0
setup (thor) 11 11 0
setup (roxie) 11 11 0
test (hthor) 734 734 0
test (thor) 647 647 0
test (roxie) 761 761 0

HPCC Stop: OK
HPCC Uninstall: OK

@richardkchapman richardkchapman merged commit decd15d into hpcc-systems:master Dec 19, 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.