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

Cython enums for null_equality and nan_equality redeclare 'UNEQUAL', raising a warning during builds. #9462

Closed
bdice opened this issue Oct 18, 2021 · 3 comments · Fixed by #12947
Labels
Python Affects Python cuDF API.

Comments

@bdice
Copy link
Contributor

bdice commented Oct 18, 2021

The Cython enums null_equality and nan_equality both have an enum value named UNEQUAL.

ctypedef enum null_equality "cudf::null_equality":
EQUAL "cudf::null_equality::EQUAL"
UNEQUAL "cudf::null_equality::UNEQUAL"
ctypedef enum nan_equality "cudf::nan_equality":
ALL_EQUAL "cudf::nan_equality::ALL_EQUAL"
UNEQUAL "cudf::nan_equality::UNEQUAL"

When building the cuDF Python code, I see an error message repeated many times:

warning: cudf/_lib/cpp/types.pxd:51:8: 'UNEQUAL' redeclared

This issue of repeated enum names has been previously discussed on StackOverflow: https://stackoverflow.com/questions/50653853/duplicate-enum-member-names-in-cython-redeclaration-error

Looking at the compiled C++, I see that this Cython code...

cdef null_equality c_nulls_equal = (
null_equality.EQUAL if nulls_equal else null_equality.UNEQUAL
)
cdef nan_equality c_nans_equal = (
nan_equality.ALL_EQUAL if nans_all_equal else nan_equality.UNEQUAL
)

... translates into this C++, which seems fine.

  /* "cudf/_lib/lists.pyx":91
 *     )
 *     cdef null_equality c_nulls_equal = (
 *         null_equality.EQUAL if nulls_equal else null_equality.UNEQUAL             # <<<<<<<<<<<<<<
 *     )
 *     cdef nan_equality c_nans_equal = (
 */
  if ((__pyx_v_nulls_equal != 0)) {
    __pyx_t_3 = cudf::null_equality::EQUAL;
  } else {
    __pyx_t_3 = cudf::null_equality::UNEQUAL;
  }
  __pyx_v_c_nulls_equal = __pyx_t_3;
  /* "cudf/_lib/lists.pyx":94
 *     )
 *     cdef nan_equality c_nans_equal = (
 *         nan_equality.ALL_EQUAL if nans_all_equal else nan_equality.UNEQUAL             # <<<<<<<<<<<<<<
 *     )
 *
 */
  if ((__pyx_v_nans_all_equal != 0)) {
    __pyx_t_4 = cudf::nan_equality::ALL_EQUAL;
  } else {
    __pyx_t_4 = cudf::nan_equality::UNEQUAL;
  }
  __pyx_v_c_nans_equal = __pyx_t_4;

Therefore, the issue that the warning is trying to alert us about is not a problem because of our use of explicit C names in the pxd file, like "cudf::nan_equality::UNEQUAL".

Discussing with @shwina, we identified two paths forward to remove this warning:

  1. Wait for Cython 3.0, and start using enum class which does not have this scoping issue.
  2. Prefix the enum's Cython names to something like NAN_ALL_EQUAL, NAN_UNEQUAL.

For now, @shwina and I agreed that we should take no action and wait for Cython 3.0 (option 1).

@bdice bdice added bug Something isn't working Needs Triage Need team to review and classify labels Oct 18, 2021
@bdice bdice added Cython and removed bug Something isn't working Needs Triage Need team to review and classify labels Oct 18, 2021
@vyasr
Copy link
Contributor

vyasr commented Oct 18, 2021

Yep I think we've discussed this before and come to the conclusion that waiting for scoped enum support in Cython is fine unless this conflict actually starts causing problems.

@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@GregoryKimball GregoryKimball added Python Affects Python cuDF API. and removed inactive-30d labels Nov 21, 2022
rapids-bot bot pushed a commit that referenced this issue Mar 17, 2023
…arnings. (#12947)

Closes #9462.

```
warning: cudf/_lib/cpp/types.pxd:51:8: 'UNEQUAL' redeclared
```

We've been dealing with warnings like this for as long as I've worked on cudf. I think it'd be good to fix this by renaming the Cython enum. We don't rely on this name very much at the Cython/Python level, so it's a small change. (This has been irking me for a long time and I want to see this warning go away in the build logs.)

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - https://github.com/brandon-b-miller

URL: #12947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants