Skip to content

Fix -- Remove sort of feature keys from preprocessed metadata FeatureSpecDict#306

Merged
mkolodner-sc merged 5 commits intomainfrom
mkolodner-sc/fix_preprocessed_metdata_pb_bug
Sep 3, 2025
Merged

Fix -- Remove sort of feature keys from preprocessed metadata FeatureSpecDict#306
mkolodner-sc merged 5 commits intomainfrom
mkolodner-sc/fix_preprocessed_metdata_pb_bug

Conversation

@mkolodner-sc
Copy link
Copy Markdown
Collaborator

@mkolodner-sc mkolodner-sc commented Sep 2, 2025

Scope of work done

  • Removes the sorted calls operating on the feature and label keys, which was causing training failures with the FeatureEmbeddingLayer
  • Add test to ensure that the proto feature keys and the feature keys from the pb wrapper are the same in value and order.

Where is the documentation for this feature?: N/A

Did you add automated tests or write a test plan?

Updated Changelog.md? NO

Ready for code review?: NO

@mkolodner-sc mkolodner-sc changed the title Potential Fix: PreprocessedMetadata FeatureKeys bug Preprocessed Metadata FeatureKeys Fix Sep 3, 2025
@mkolodner-sc mkolodner-sc changed the title Preprocessed Metadata FeatureKeys Fix Fix -- Remove sort of feature keys from preprocessed metadata FeatureSpecDict Sep 3, 2025
Copy link
Copy Markdown
Collaborator

@kmontemayor2-sc kmontemayor2-sc left a comment

Choose a reason for hiding this comment

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

Thanks Matt, excellent digging :)

Comment thread python/gigl/src/common/types/pb_wrappers/preprocessed_metadata.py
@svij-sc
Copy link
Copy Markdown
Collaborator

svij-sc commented Sep 3, 2025

Some of these keys that are sorted don't have only one access pattern.
Thus, i'd imagine if they are sorted in one place but not in other places it can easily cause issues.

Screenshot 2025-09-03 at 11 17 12 AM

@mkolodner-sc mkolodner-sc marked this pull request as ready for review September 3, 2025 18:31
@mkolodner-sc mkolodner-sc added this pull request to the merge queue Sep 3, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 3, 2025
@mkolodner-sc mkolodner-sc added this pull request to the merge queue Sep 3, 2025
Merged via the queue into main with commit 9d42b42 Sep 3, 2025
4 checks passed
@mkolodner-sc mkolodner-sc deleted the mkolodner-sc/fix_preprocessed_metdata_pb_bug branch September 3, 2025 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants