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

Custom Checkpoint file extension #4963

Closed
Borda opened this issue Dec 3, 2020 · 9 comments · Fixed by #4977
Closed

Custom Checkpoint file extension #4963

Borda opened this issue Dec 3, 2020 · 9 comments · Fixed by #4977
Assignees
Labels
checkpointing Related to checkpointing feature Is an improvement or enhancement good first issue Good for newcomers

Comments

@Borda
Copy link
Member

Borda commented Dec 3, 2020

🚀 Feature

Atm, we have hardcoded .ckpt as a file extension for any checkpoint.

https://github.com/PyTorchLightning/pytorch-lightning/blob/db69d169e868451c16a4ee418f2de835ccce146c/pytorch_lightning/callbacks/model_checkpoint.py#L429

Proposed solution 1:

class ModelCheckpoint(Callback):
    FILE_EXTENSION = ".ckpt"

Proposed solution 2:

ModelCheckpoint(ext=''pt')
@Borda Borda added feature Is an improvement or enhancement help wanted Open to be worked on good first issue Good for newcomers checkpointing Related to checkpointing labels Dec 3, 2020
@edenlightning edenlightning changed the title Custom Checkpoint extension Custom Checkpoint file extension Dec 3, 2020
@awaelchli
Copy link
Contributor

Should it be part of the filename?
Adding a new argument would require some justification I think.

@carmocca
Copy link
Contributor

carmocca commented Dec 3, 2020

Some previous discussion about this: #3163 (comment)

@edenlightning
Copy link
Contributor

I agree with @awaelchli, but just need to handle some edge cases (don;t think we should support "." in filenames anway, no?)

@Borda
Copy link
Member Author

Borda commented Dec 3, 2020

I agree with @awaelchli, but just need to handle some edge cases (don;t think we should support "." in filenames anway, no?)

If we define it as propose in the description, all behaviors stay the same for now... And if use overwrite it is, it is his decision and responsibility

@rohitgr7
Copy link
Contributor

rohitgr7 commented Dec 4, 2020

Should it be part of the filename?
Adding a new argument would require some justification I think.

I thought so too, but what if user wants the default filename created by PL but with a different extension? In that case it should be filename=None but one can still set different ckpt_extension.

@Borda
Copy link
Member Author

Borda commented Dec 4, 2020

well in such case you need to pass the instance of checkpoint call back anyway, so I would fo keeping untouched API inherit it...

class ModelCheckpoint(Callback):
    FILE_EXTENSION = ".ckpt"
    ...

class MyModelCheckpoint(ModelCheckpoint):
    FILE_EXTENSION = ".abc"  # only this line, nothing else is needed

so you can later pass [ModelCheckpoint(), MyModelCheckpoint()]

@Borda
Copy link
Member Author

Borda commented Dec 4, 2020

I agree with @awaelchli, but just need to handle some edge cases (don;t think we should support "." in filenames anway, no?)

as you agree with it I have reverted the description to the original one :]

@rohitgr7
Copy link
Contributor

rohitgr7 commented Dec 4, 2020

Proposed solution 1:
class ModelCheckpoint(Callback):
FILE_EXTENSION = ".ckpt"

I'd suggest this one

@janhenriklambrechts
Copy link
Contributor

Ill happily work on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checkpointing Related to checkpointing feature Is an improvement or enhancement good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants