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

Remove legacy Photon code #456

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ashelkovnykov
Copy link
Contributor

  • Remove legacy Driver class used for training fixed effect models only
  • Remove all code used only by the above driver
  • Modify AvroDataReader and AvroUtils to load features as NameTermValueAvro objects, instead of GenericRecord objects and then expecting to find certain fields by name

- Remove legacy Driver class used for training fixed effect models only
- Remove all code used only by the above driver
- Modify AvroDataReader and AvroUtils to load features as NameTermValueAvro objects, instead of GenericRecord objects and then expecting to find certain fields by name
@ashelkovnykov
Copy link
Contributor Author

@cmjiang @yunboouyang @lguo
Updated for the latest master, requesting review

@joshvfleming
Copy link
Contributor

Regarding:

Modify AvroDataReader and AvroUtils to load features as NameTermValueAvro objects, instead of GenericRecord objects and then expecting to find certain fields by name

IIRC, the reason we did it this way was that a lot of teams were still using NameTermValueAvro-like schemas to encode feature values, but not exactly NameTermValueAvro.

@ashelkovnykov
Copy link
Contributor Author

@joshvfleming
My proposed changes (in AvroUtils) are actually specifically looking out for this case. I ran into an issue where if a GenericRecord matched the input schema, but it wasn't the exact output schema, then the cast would fail. So now, we look for all of the schema fields by name but in a generic way where the names aren't hard-coded like before.

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.

2 participants