Skip to content

Unify the logics to check eager mode#7709

Merged
JackCaoG merged 4 commits intomasterfrom
JackCaoG/unify_UseEagerDebugMode
Jul 19, 2024
Merged

Unify the logics to check eager mode#7709
JackCaoG merged 4 commits intomasterfrom
JackCaoG/unify_UseEagerDebugMode

Conversation

@JackCaoG
Copy link
Copy Markdown
Collaborator

Prior to my eager mode changes, eager mode is a debug only mode that's controlled by the XLA_USE_EAGER_DEBUG_MODE env var, now since we want to make it more offical I want to control it explictly with the API instead of the env var. @aws-rhsoln let me know what you think. Eventually I want to deprecate the env var(at least not call it debug mode).

@JackCaoG JackCaoG added the eager PyTorch/XLA eager-mode label Jul 17, 2024
@aws-rhsoln
Copy link
Copy Markdown
Contributor

Do you think its valuable to add a message letting the users know that eager mode run without torch.compile is going to be slow, can provide some basic reasoning as to why that would be the case.
Need to let users know that graph based mode is still going to be faster (be it using compile or lazy eval).

@JackCaoG
Copy link
Copy Markdown
Collaborator Author

@aws-rhsoln check my doc at #7700, I am still thinking about how to message users better to let them know eager mode exists. but yea I agree that we have to make it clear that compile is the way to go for perfomrance.

@JackCaoG JackCaoG marked this pull request as ready for review July 19, 2024 17:39
@JackCaoG
Copy link
Copy Markdown
Collaborator Author

@aws-rhsoln for some reason I can't make you a reviewer, do you mind also taking a look at this pr?

Comment thread torch_xla/__init__.py
plugins.use_dynamic_plugins()
plugins.register_installed_plugins()

if os.getenv('XLA_USE_EAGER_DEBUG_MODE', '0') == '1':
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this just for backward compatibility, since we already have the api now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yea, it is mostly for backward compatibility. I think I can also tweak it and use it in test to test both eager and non-eager.

Comment thread torch_xla/csrc/tensor.cpp Outdated
graph_executor->RegisterTensor(xtensor->data());
if ((UseEagerDebugMode() || graph_executor->UseEagerMode()) &&
!delay_eager_executation) {
if ((graph_executor->UseEagerMode()) && !delay_eager_executation) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Extra brackets around graph_executor->UseEagerMode()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

nice catch

Copy link
Copy Markdown
Contributor

@aws-rhsoln aws-rhsoln left a comment

Choose a reason for hiding this comment

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

Just one nit, lgtm otherwise! Thanks!

Copy link
Copy Markdown
Collaborator

@alanwaketan alanwaketan left a comment

Choose a reason for hiding this comment

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

LGTM.

@JackCaoG JackCaoG merged commit 4ba63ff into master Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

eager PyTorch/XLA eager-mode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants