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

[AIR] Framework specific Checkpoint API consistency #28412

Closed
amogkam opened this issue Sep 9, 2022 · 13 comments
Closed

[AIR] Framework specific Checkpoint API consistency #28412

amogkam opened this issue Sep 9, 2022 · 13 comments
Assignees
Labels
@external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. P1 Issue that should be fixed within a few weeks

Comments

@amogkam
Copy link
Contributor

amogkam commented Sep 9, 2022

We should make sure all of our framework-specific Checkpoints follow similar API design patterns.

  • For creating checkpoints via different sources (for example, from a model object, from a path, etc.), these should be expressed as different methods and not overloaded onto the same method.
  • All framework specific checkpoint classes should expose def get_model(self, *args, **kwargs) and def get_preprocessor(self) -> Optional[Preprocessors] methods. These can be defined in a private abstract base class that all framework specific checkpoint can inherit from.
  • All predictors should interact with framework specific checkpoints through get_model and get_preprocessor APIs.
  • The framework-specific Predictors' from_checkpoint method should accept **get_model_kwargs so users can specify kwargs that should be passed through to checkpoint.get_model(...)

cc @xwjiang2010

@heyitsmui
Copy link
Contributor

heyitsmui commented Sep 13, 2022

@xwjiang2010 @amogkam @krfricke im happy to take a look at this if this hasn't been grabbed!

@richardliaw
Copy link
Contributor

yeah please do so! and keep us updated. opening a draft PR would be the fastest way to collaborate here!

@amogkam
Copy link
Contributor Author

amogkam commented Sep 20, 2022

hey @heyitsmui, let me know if you are still planning to work on this :).

@heyitsmui
Copy link
Contributor

hey @xwjiang2010 i believe you mentioned you already have a draft PR for this? if not, i can get started on this :)

@xwjiang2010
Copy link
Contributor

Ah the one I have is this: #28474
It's currently being blocked by another PR, which should be landed soon. I dont have other parts of this task planned yet.

@heyitsmui
Copy link
Contributor

ah thanks @xwjiang2010 for the clarification! i will take a stab at the rest of the tasks planned, on top of your current PR, thanks!

@amogkam
Copy link
Contributor Author

amogkam commented Sep 20, 2022

Thanks @heyitsmui! Let me know if you have any questions!

@heyitsmui
Copy link
Contributor

Working on this now, expect to have a draft PR out for review later today

@richardliaw
Copy link
Contributor

richardliaw commented Sep 27, 2022 via email

@richardliaw
Copy link
Contributor

@heyitsmui did you create a pr for this?

@richardliaw
Copy link
Contributor

bump @heyitsmui ?

@heyitsmui
Copy link
Contributor

heyitsmui commented Nov 16, 2022

@richardliaw thanks for checking in, let me clean up my change, will have a PR out for review by tomorrow EOD

@richardliaw
Copy link
Contributor

^^ @heyitsmui :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. P1 Issue that should be fixed within a few weeks
Projects
None yet
Development

No branches or pull requests

5 participants