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

[REVIEW] libgdf_cffi renamed to libcudf_cffi #1343

Closed
wants to merge 5 commits into from

Conversation

pradghos
Copy link
Contributor

@pradghos pradghos commented Apr 3, 2019

This PR is addressing #1341

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@pradghos pradghos changed the title [WIP] libgdf_cffi remaned to libcudf_cffi [WIP] libgdf_cffi renamed to libcudf_cffi Apr 3, 2019
@harrism
Copy link
Member

harrism commented Apr 4, 2019

We plan to replace CFFI with Cython in 0.7 release so this may be redundant. Suggest starting by filing an issue in the future to discuss and save effort. See #599

@harrism harrism added the 2 - In Progress Currently a work in progress label Apr 4, 2019
@kkraus14
Copy link
Collaborator

kkraus14 commented Apr 4, 2019

ok to test

@kkraus14 kkraus14 added Python Affects Python cuDF API. Cython labels Apr 4, 2019
@pradghos pradghos changed the title [WIP] libgdf_cffi renamed to libcudf_cffi [REVIEW] libgdf_cffi renamed to libcudf_cffi Apr 4, 2019
@pradghos
Copy link
Contributor Author

pradghos commented Apr 4, 2019

I know cffi related changes will be removed probably once - issue#599 gets in. However, I am still marking as [REVIEW] - So that it can be merged as intermediate patch. Please let me know in case any comments. Thank you !

@harrism harrism added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Apr 5, 2019
@pradghos
Copy link
Contributor Author

pradghos commented Apr 8, 2019

Please keep me posted in case of any further review comments / suggestion . Thank you!

# with pytest.raises(GDFError) as raises:
# libcudf.gdf_add_generic(col, col, col)

# raises.match("CUDA ERROR.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the goal of this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kkraus14 : This is existing test case only ! Probably after renaming the path from libgdf_cffi to libcudf_cffi, it is showing differently. We can see there is another test_errorhandling.py being marked as deleted also.

So, there is nothing change as functionality is concern - Like all the places in code drop libcudf_cffi and libcudf is updated to pick up the intended module name. Thank you !

@harrism
Copy link
Member

harrism commented Apr 9, 2019

@kkraus14 Do we need to move this PR forward if we are just going to rip CFFI out in this release? If not, I recommend closing.

@kkraus14
Copy link
Collaborator

kkraus14 commented Apr 9, 2019

@kkraus14 Do we need to move this PR forward if we are just going to rip CFFI out in this release? If not, I recommend closing.

Let's leave this in our back pocket in case that slips unexpectedly, so lets leave this open until then.

@kkraus14 kkraus14 added 0 - Blocked Cannot progress due to external reasons and removed 3 - Ready for Review Ready for review by team labels Apr 9, 2019
@harrism
Copy link
Member

harrism commented May 2, 2019

Looks like @kkraus14 has replaced nearly all CFFI with Cython and will complete the task in this release, so closing this PR. Thanks @pradghos and sorry for the waste of your effort.

To avoid that in the future, start a discussion in an issue first. I think you did file an issue, but needed to wait longer for a response before starting the work.

@harrism harrism closed this May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Blocked Cannot progress due to external reasons Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants