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.

Sign up for GitHub

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

Jump to bottom

Criteo dataloader #642

Merged
merged 38 commits into from
Mar 19, 2019
Merged

Criteo dataloader #642

merged 38 commits into from
Mar 19, 2019

Conversation

miguelgfierro
Copy link
Collaborator

Description

Criteo dataload in python and spark
Smoke and integration

Related Issues

#555

Checklist:

  • My code follows the code style of this project, as detailed in our contribution guidelines.
  • I have added tests.
  • I have updated the documentation accordingly.

jreynolds01 and others added 29 commits February 22, 2019 16:25
@jreynolds01
initialize criteo dataloader from movielens
f525385
@jreynolds01
create v1 criteo_dac to run on databricks
861ba28
@jreynolds01
Merge branch 'staging' into jeremr-criteo-dataloader
30980c2
@jreynolds01
add dbutils to _load_datafile
6aa0272
@jreynolds01
Update:
5d8471d 
.github
.gitignore
AUTHORS.md
CONTRIBUTING.md
LICENSE
README.md
SETUP.md
notebooks
reco_utils
scripts
tests
@jreynolds01
update message in _load_datafile
70dcaa7
@jreynolds01
check for local_cache_path before copying from dbfs as well
6c02522
@jreynolds01
update path to check
9d848f4
@jreynolds01
remove archive capability
0d4c7d3
@jreynolds01
cleanup residuals from movielens
573173b
@jreynolds01
first pass lightgbm estimator
92049c6
@jreynolds01
update
9f41221
@jreynolds01
Merge branch 'staging' into jeremr-criteo-dataloader
59dc3bf
@jreynolds01
move _clean_up to a more general place
460cce1
@jreynolds01
address #574
7d6488a
@jreynolds01
fix refactor of _clean_up
d41cff3
@jreynolds01
update
bb986bc
@jreynolds01
address missing value propagation
58ae951
@jreynolds01
update persisting the file
048dd8d
@jreynolds01
update for missing values issue
97dd692
@miguelgfierro
adapted to spark
cbda0ae
@miguelgfierro
added sample dataset and refactored to follow single responsibility
4312b22
@miguelgfierro
refactored and documented
a4b401b
@miguelgfierro
rename tests for datasets
cce6983
@miguelgfierro
smoke and integration tests
92b6313
@miguelgfierro
🐛
a8d75b6
@miguelgfierro
merge with staging and fix conflicts
5000198
@miguelgfierro
update criteo dataset
8f5354a
@miguelgfierro
Merge pull request #619 from Microsoft/jeremr-criteo-dataloader-addition
5236c60 
Jeremr criteo dataloader addition
motefly
motefly reviewed Mar 13, 2019
View reviewed changes
Copy link
Contributor

@motefly motefly left a comment

Choose a reason for hiding this comment

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

Looks great.

gramhagen
gramhagen requested changes Mar 13, 2019
View reviewed changes
Copy link
Collaborator

@gramhagen gramhagen left a comment

Choose a reason for hiding this comment

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

sorry that you have to bear the brunt of my grumpiness with how the data loading is setup =)
we can simplify this section and avoid creating unnecessary single use functions or convenience classes that make the code more brittle to changes to dependencies. this will also limit the test surface.

reco_utils/common/python_utils.py Outdated Show resolved Hide resolved
reco_utils/dataset/criteo_dac.py Outdated Show resolved Hide resolved
reco_utils/dataset/criteo_dac.py Outdated Show resolved Hide resolved
reco_utils/dataset/criteo_dac.py Outdated Show resolved Hide resolved
reco_utils/dataset/criteo_dac.py Outdated Show resolved Hide resolved
reco_utils/dataset/criteo_dac.py Outdated Show resolved Hide resolved
reco_utils/dataset/movielens.py Outdated Show resolved Hide resolved
reco_utils/dataset/url_utils.py Show resolved Hide resolved
notebooks/00_quick_start/lightgbm_tinycriteo.ipynb Show resolved Hide resolved
reco_utils/dataset/criteo_dac.py Outdated Show resolved Hide resolved
loomlike
loomlike reviewed Mar 13, 2019
View reviewed changes
Copy link
Collaborator

@loomlike loomlike left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Check my comment for potential misbehavior that might be caused by removing the clean-up codes.

reco_utils/dataset/criteo_dac.py Show resolved Hide resolved
miguelgfierro
miguelgfierro commented Mar 18, 2019
View reviewed changes
reco_utils/dataset/criteo.py Outdated


@contextmanager
def _real_path(path):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@loomlike we should do DRY here. However, this code is a little bit different to what you have in movielens. I'm not sure how to homogenize

Copy link
Collaborator

Choose a reason for hiding this comment

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

I got it. What about this:

  1. Put this real_path to url_utils (btw we talked about renaming this to dataset_utils, right?)
  2. Use that function from criteo.py
  3. Refactor movielens to use the same function. Probablly, to check "zip" file name from outside of this function. - either you or I can refactor this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks reasonable to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. @loomlike if you are free today, feel free to push directly to the branch. If not I'll do it tomorrow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@loomlike I added an issue for the renaming #662

gramhagen
gramhagen approved these changes Mar 18, 2019
View reviewed changes
Copy link
Collaborator

@gramhagen gramhagen left a comment

Choose a reason for hiding this comment

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

cool, let's merge this!

reco_utils/dataset/criteo.py Outdated


@contextmanager
def _real_path(path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks reasonable to me

@miguelgfierro miguelgfierro mentioned this pull request Mar 19, 2019
Closed
@miguelgfierro miguelgfierro merged commit df96631 into staging Mar 19, 2019
@miguelgfierro miguelgfierro deleted the jeremr-criteo-dataloader branch March 19, 2019 22:02
@miguelgfierro miguelgfierro mentioned this pull request Mar 24, 2019
Closed
yueguoguo pushed a commit that referenced this pull request Sep 9, 2019
@miguelgfierro
Merge pull request #642 from Microsoft/jeremr-criteo-dataloader
6047ea2 
Criteo dataloader
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loomlike loomlike loomlike left review comments

@motefly motefly motefly left review comments

@anargyri anargyri anargyri approved these changes

@gramhagen gramhagen gramhagen approved these changes

@yueguoguo yueguoguo Awaiting requested review from yueguoguo

@jreynolds01 jreynolds01 Awaiting requested review from jreynolds01

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@miguelgfierro @gramhagen @motefly @anargyri @loomlike @jreynolds01