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.

Sign up for GitHub

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

use "true" and "false" instead of 0 and 1 #890

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

use "true" and "false" instead of 0 and 1 #890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ec41fb
use "true" and "false" instead of 0 and 1
samos123 Dec 12, 2024
9789066
Merge branch 'main' into no-01-boolean
samos123 Jan 17, 2025
b69fa47
update tests
samos123 Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion axlearn/common/compiler_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def xla_flags_from_options(xla_options: dict[str, Union[str, bool, int]]) -> str
flags = []
for k, v in xla_options.items():
if isinstance(v, bool):
v = "1" if v else "0"
v = "true" if v else "false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we convert all the "true" and "false" strings in default_xla_options to bools to be consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that should be preferred. I just wanted to keep this PR small to make merging and reviewing easier. Happy to include that change in this PR or do a follow up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I think it makes sense to include in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to do an aot compilation or run the trainer for a few steps to confirm these changes work? (If you haven't already?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did test it on v6e afair last year, but any additional runs would be great.

Copy link
Contributor

@apghml apghml Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe test it again on v5p using the jax/jaxlib version that is currently pinned in AXLearn? (0.4.33 IIRC)

Copy link
Contributor

Choose a reason for hiding this comment

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

Were you able to run this?

flags.append(f"--{k}={v}")
apghml marked this conversation as resolved.
Show resolved Hide resolved
return " ".join(flags)

Expand Down
4 changes: 2 additions & 2 deletions axlearn/common/compiler_options_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ def f(x: Tensor) -> Tensor:
)
self.assertEqual(f_compiled(5), 15)

def atest_xla_flags_from_options(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test was never being run because of the a before test in function name.

def test_xla_flags_from_options(self):
options = dict(a="true", b="false", c=True, d=False, long_option_name=True)
result = compiler_options.xla_flags_from_options(options)
self.assertEqual(result, "--a=true --b=false --c=1 --d=0 --long_option_name=1")
self.assertEqual(result, "--a=true --b=false --c=true --d=false --long_option_name=true")

def test_xsc_compiler_options(self):
options = compiler_options.infer_xsc_compiler_options(
Expand Down
Loading