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

[BUG] Exporting embedding tables to parquet does not match the TF variable weights #406

Closed
gabrielspmoreira opened this issue Apr 28, 2022 · 4 comments · Fixed by #452
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@gabrielspmoreira
Copy link
Member

Bug description

When we export the embedding table of a categorical feature to a dataframe (embedding_table_df()), dataset (embedding_table_dataset()) or parquet file (export_embedding_table()) the values in the exported dataframe/parquet do not match the values of the original feature embedding weighs.

Steps/Code to reproduce bug

  1. Disable skipping the test test_embedding_features_exporting_and_loading_pretrained_initializer() in tests\tf\features\test_embeddings.py
  2. That test will fail because EmbeddingFeatures.embedding_table_dataset() uses under the hood cudf.from_dlpack(), which is not working as expected we reported in this issue #10754 of cudf repo.

Expected behavior

The values of the exported embedding table should match the embedding table weights.

@gabrielspmoreira
Copy link
Member Author

This issue is blocked by issue #10754 of cudf repo.

@gabrielspmoreira gabrielspmoreira changed the title [BUG] Exporting embedding tables to parquet does not match the weights of the embedding tables [BUG] Exporting embedding tables to parquet does not match the TF variable weights Apr 28, 2022
@viswa-nvidia
Copy link

@gabrielspmoreira , what is the impact of this bug - Which example is this affecting ? @EvenOldridge for viz.

@EvenOldridge
Copy link
Member

@gabrielspmoreira we may be assuming cudf behaves in a specific way. I believe that order of the list isn't guaranteed unless you specifically order it. There may be a way to force that.

@gabrielspmoreira
Copy link
Member Author

Sure @EvenOldridge. The cudf team mentioned in my issue that cudf.from_dlpack() expects tensor to be in column-major (Fortran order). The cudf team placed a PR based on that to raise an exception if tensor is not in column-major.
From I what investigated, TF doesn't support encoding tensors in Fortran order (like numpy and cupy do).
So I fixed the issue on our side in this PR by converting from TF to cuPy using DLPack and then creating the cudf dataframe from cuPy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants