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

Read manifest inside _infer_with_profiler method #2480

Closed
pedrogengo opened this issue Jul 19, 2023 · 5 comments · Fixed by #2488
Closed

Read manifest inside _infer_with_profiler method #2480

pedrogengo opened this issue Jul 19, 2023 · 5 comments · Fixed by #2488
Labels
triaged Issue has been reviewed and triaged

Comments

@pedrogengo
Copy link
Contributor

🚀 The feature

Add a line to read the manifest file inside the method _infer_with_profiler if self.manifest is None (this can happen in cases the user overwrites the initialize method).

If you think this makes sense, Im more than happy to contribute with it :)

Motivation, pitch

I'm working in some projects where we are overwriting the initialize method from base handler and we weren't creating the self.manifest attribute on it. When I tried to run the torch profiler, the request broke because self.manifest was None. IMO, as this attribute is used for other methods we should guarantee that we will have that or if the attribute is None we can read the file inside the _infer_with_profiler method.

This issue will happen to some examples too, like this one.

Alternatives

  • If self.manifest is None, read the file inside _infer_with_profiler;
  • Update documentation to be explicit that you need to have the self.manifest defined in your initialize method.

Additional context

No response

@agunapal
Copy link
Collaborator

Hi @pedrogengo Thanks for the proposal. I think it makes sense. Please feel free to send a PR.

@agunapal agunapal added the triaged Issue has been reviewed and triaged label Jul 19, 2023
@pedrogengo
Copy link
Contributor Author

Tomorrow I will open a PR for that, tks for your quick reply

@pedrogengo
Copy link
Contributor Author

@agunapal I opened a PR for this :)

@agunapal
Copy link
Collaborator

Thanks! Will review it tomorrow

@pedrogengo
Copy link
Contributor Author

I decided to don't change anything in the _infer_with_profiler, but delegate the manifest checking to handle, as it is the method that call the _infer_with_profiler. Let me know what you think :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged Issue has been reviewed and triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants