Skip to content

Document environment variables #4273

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

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Document environment variables #4273

merged 1 commit into from
Dec 8, 2022

Conversation

steventk-g
Copy link
Collaborator

@steventk-g steventk-g commented Dec 5, 2022

Created a yaml file that captures all relevant environment variables with description, type, and default value.

Variables are grouped into the following:

  • Build variables
  • Debug variables
  • Device variables
  • PJRT variables
  • XRT variables
  • Feature variables

- Verbosity level for GRPC, e.g. INFO, ERROR, etc.
type: string
default_value: "ERROR"
TPU_ML_PLATFORM:
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol I felt like we should not make this configurable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if we should include it here for the sake of documentation, and remark that it shouldn't be changed. My intention in including it was just to cover every environment variable that shows up in this repo.

_set_missing_env('TPU_ML_PLATFORM', 'PyTorch/XLA')

Copy link
Collaborator

@JackCaoG JackCaoG left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Thanks for putting this together! Is it possible to group similar env var for better reading expericence?

For example we can split them into

  1. XRT related
  2. Build related
  3. Debug related
  4. Feature enablement
  5. Device configuration
  6. Distributed related
  7. ...

Copy link
Collaborator

@will-cromar will-cromar left a comment

Choose a reason for hiding this comment

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

Wow! Thanks for laying all of this out. LGTM at least for the variables I know about

I agree with Jack's comment that it would be clearer if you categorized the variables, especially separating the build-time variables from the run-time variables and separating the XRT configuration from the rest.

@JackCaoG Do we also want some designation of which variables are stable and which might get removed in the future? Or are we committed to actually supporting all of these?

@JackCaoG
Copy link
Collaborator

JackCaoG commented Dec 7, 2022

yea I think in this or I am fine with only group them by cateratory. We can update it based on stable/to_be_removed in a follow up pr.

Copy link
Collaborator

@JackCaoG JackCaoG left a comment

Choose a reason for hiding this comment

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

Thanks!

@steventk-g steventk-g marked this pull request as ready for review December 8, 2022 01:13
@steventk-g steventk-g merged commit 36c53d6 into master Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants