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

[feature] Add checkpoint methods to MT/Table #5528

Merged
merged 1 commit into from Mar 6, 2019

Conversation

Projects
None yet
4 participants
@tpoterba
Copy link
Collaborator

commented Mar 4, 2019

No description provided.

@cseed
Copy link
Collaborator

left a comment

Want to discuss interface for this. Should we checkpoint to scratch space by default (e.g. path is optional?)

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

Konrad asked the same question, I told him we can add that feature back-compatibly, but it's not trivial.

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

right now that no-args thing is persist()

@konradjk

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

I'm actually not sure it's exactly persist. I think persist would hold some in memory and spill to disk, right? Or does it actually write the whole thing?

@cseed

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

we can add that feature back-compatibly, but it's not trivial

It's not trivial? Then shouldn't we figure it out now? persist is a totally different beast. I think we'd both argue the users shouldn't have to know the difference between these things. Should persist just write/read?

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

sorry, wasn't clear. I don't think it's trivial to figure out what a no-args checkpoint should do, but it IS trivial to make that change back-compatibly when we do.

@cseed

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

Wait, what's wrong with just getting a temporary path with new_temp_file?

@konradjk

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

that's what I suggested I almost suggested until I was shot down 😄

@cseed

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

Tim, can you shoot me down, too?

@tpoterba

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2019

ugh, I'm still exhausted from shooting Konrad down.

@cseed cseed dismissed their stale review Mar 6, 2019

Unblocking. Let's add tmp file checkpoint later (or have persist do it)

@danking danking merged commit 1d0ea65 into hail-is:master Mar 6, 2019

1 check passed

hail-ci-0-1 successful build
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.