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

[Torch] platform alibaba bazel configuration #210

Merged
merged 4 commits into from
Apr 7, 2022

Conversation

tanyokwok
Copy link
Collaborator

@tanyokwok tanyokwok commented Mar 30, 2022

This pull request adds platform alibaba bazel configuration:

  1. add config option platfrom_alibaba
  2. add bazel_target_cfg.txt, so one can add extra libraries to torch_blade wheel
  3. add //bazel:platform_alibaba/workspace.bzl, which can be overrided
  4. delete common_utils/version.h

for fpath in [ext_so_fpath, ral_so_fpath, disc_bin_fpath]:
fpath = os.path.realpath(os.path.join(bazel_bin_dir, fpath))
for fpath in open("bazel_target_cfg.txt"):
fpath = os.path.realpath(os.path.join(bazel_bin_dir, fpath.strip()))
fname = os.path.basename(fpath)
_symlink_force(fpath, os.path.join(extdir, fname))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better check if these libraries do exist using os.path.exists

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -47,7 +47,7 @@ def __init__(self, *args, **kwargs):
]
torch_major_version, torch_minor_version = self.torch_version.split(".")[:2]
self.extra_opts = [
"--copt=-DPYTORCH_VERSION_STRING={}".format(self.torch_version),
"--copt=-DPYTORCH_VERSION_STRING=\\\"{}\\\"".format(self.torch_version),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why need \\\

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We had to use \\\ to quote strings. We had run out of the problem before since we had never use it.


for fpath in [ext_so_fpath, ral_so_fpath, disc_bin_fpath]:
fpath = os.path.realpath(os.path.join(bazel_bin_dir, fpath))
for fpath in open("bazel_target_cfg.txt"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want users to extend this configuration file? If not, maybe some const string instead of an additional text file is enough.
If we want users to do something on the configuration file, we also need a guide doc to tell them how to do that.

Copy link
Collaborator Author

@tanyokwok tanyokwok Apr 1, 2022

Choose a reason for hiding this comment

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

As comments said, I added some comments here. It does want users to extend it, but those users are mainly ourselves. So I don't want to add it to documentation since it might not be ready for users. A user is recommended to use the default setting.

if_tensorrt_enabled(["-DTORCH_BLADE_BUILD_TENSORRT"])
if_tensorrt_enabled(["-DTORCH_BLADE_BUILD_TENSORRT"]) +
if_platform_alibaba(
["-DTORCH_BLADE_COMMUNITY_VERSION=false"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, version means a number? The bool value is confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, renaming to TORCH_BLADE_PLATFORM_ALIBABA

Copy link
Collaborator

@Orion34-lanbo Orion34-lanbo left a comment

Choose a reason for hiding this comment

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

LGTM

@tanyokwok tanyokwok merged commit 097673e into main Apr 7, 2022
@tanyokwok tanyokwok deleted the features/platform_alibaba branch April 7, 2022 01:28
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.

5 participants