[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18222: Fix PyPI Packages and Python Docker Images nightly release
marcoabreu commented on a change in pull request #18222: URL: https://github.com/apache/incubator-mxnet/pull/18222#discussion_r419317553 ## File path: cd/utils/artifact_repository.py ## @@ -396,7 +396,7 @@ def get_s3_key_prefix(args: argparse.Namespace, subdir: str = '') -> str: :param subdir: An optional subdirectory in which to store the files. Post-pended to the end of the prefix. :return: A string containing the S3 key prefix to be used to uploading and downloading files to the artifact repository """ -prefix = "{git_sha}/{libtype}/{os}/{variant}/".format(**vars(args)) +prefix = "{git_sha}/{libtype}/{variant}/".format(**vars(args)) Review comment: My concern here is that we'd be changing the directory structure in s3 - which we then have to fix later on. I'd propose to either introduce the platform identifier immediately or replace the os parameter with a matching value (replace ubuntu1804 with Ubuntu 1604) as a hotfix. But I'd prefer if we do not change the directory structure in the underlying system as that'd be something we have to clean up later. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18222: Fix PyPI Packages and Python Docker Images nightly release
marcoabreu commented on a change in pull request #18222: URL: https://github.com/apache/incubator-mxnet/pull/18222#discussion_r419183312 ## File path: cd/utils/artifact_repository.py ## @@ -396,7 +396,7 @@ def get_s3_key_prefix(args: argparse.Namespace, subdir: str = '') -> str: :param subdir: An optional subdirectory in which to store the files. Post-pended to the end of the prefix. :return: A string containing the S3 key prefix to be used to uploading and downloading files to the artifact repository """ -prefix = "{git_sha}/{libtype}/{os}/{variant}/".format(**vars(args)) +prefix = "{git_sha}/{libtype}/{variant}/".format(**vars(args)) Review comment: Okay I understand, thanks for elaborating! Last question: how do we plan to address the different platforms I mentioned as example after this PR? My understanding is that they would no longer be differentiated - speak we could only generate on platform, right? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18222: Fix PyPI Packages and Python Docker Images nightly release
marcoabreu commented on a change in pull request #18222: URL: https://github.com/apache/incubator-mxnet/pull/18222#discussion_r419073017 ## File path: cd/utils/artifact_repository.py ## @@ -396,7 +396,7 @@ def get_s3_key_prefix(args: argparse.Namespace, subdir: str = '') -> str: :param subdir: An optional subdirectory in which to store the files. Post-pended to the end of the prefix. :return: A string containing the S3 key prefix to be used to uploading and downloading files to the artifact repository """ -prefix = "{git_sha}/{libtype}/{os}/{variant}/".format(**vars(args)) +prefix = "{git_sha}/{libtype}/{variant}/".format(**vars(args)) Review comment: Creating a binary on unix, windows, arm etc will result in different binaries which have to be differentiated. It might be a valid point that the os should not be the differentiator, but it should be properly addressed (or shown) that these cases are properly addressed. When removing something, it's up to the PR creator to prove that it will not cause any harm and that the initial design decision are not broken - or otherwise explicitly state it and make an alternative proposal. "fix" is not a valid argument or proposal. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18222: Fix PyPI Packages and Python Docker Images nightly release
marcoabreu commented on a change in pull request #18222: URL: https://github.com/apache/incubator-mxnet/pull/18222#discussion_r419073017 ## File path: cd/utils/artifact_repository.py ## @@ -396,7 +396,7 @@ def get_s3_key_prefix(args: argparse.Namespace, subdir: str = '') -> str: :param subdir: An optional subdirectory in which to store the files. Post-pended to the end of the prefix. :return: A string containing the S3 key prefix to be used to uploading and downloading files to the artifact repository """ -prefix = "{git_sha}/{libtype}/{os}/{variant}/".format(**vars(args)) +prefix = "{git_sha}/{libtype}/{variant}/".format(**vars(args)) Review comment: Creating a binary on unix, windows, arm etc will result in different binaries which have to be differentiated. It might be a valid point that the os should not be the differentiator, but it should be properly addressed (or shown) that these mentioned cases are properly considered. When removing something, it's up to the PR creator to prove that it will not cause any harm and that the initial design decision are not broken - or otherwise explicitly state it and make an alternative proposal. "fix" is not a valid argument or proposal. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18222: Fix PyPI Packages and Python Docker Images nightly release
marcoabreu commented on a change in pull request #18222: URL: https://github.com/apache/incubator-mxnet/pull/18222#discussion_r419019585 ## File path: cd/utils/artifact_repository.py ## @@ -396,7 +396,7 @@ def get_s3_key_prefix(args: argparse.Namespace, subdir: str = '') -> str: :param subdir: An optional subdirectory in which to store the files. Post-pended to the end of the prefix. :return: A string containing the S3 key prefix to be used to uploading and downloading files to the artifact repository """ -prefix = "{git_sha}/{libtype}/{os}/{variant}/".format(**vars(args)) +prefix = "{git_sha}/{libtype}/{variant}/".format(**vars(args)) Review comment: I don't understand how this is a fix. The OS was put there for a reason apparently, so I'd first like to see a deep dive to understand the reasoning behind that parameter before removing it. Also, I'd like to understand how this "broke". This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18222: Fix PyPI Packages and Python Docker Images nightly release
marcoabreu commented on a change in pull request #18222: URL: https://github.com/apache/incubator-mxnet/pull/18222#discussion_r419019585 ## File path: cd/utils/artifact_repository.py ## @@ -396,7 +396,7 @@ def get_s3_key_prefix(args: argparse.Namespace, subdir: str = '') -> str: :param subdir: An optional subdirectory in which to store the files. Post-pended to the end of the prefix. :return: A string containing the S3 key prefix to be used to uploading and downloading files to the artifact repository """ -prefix = "{git_sha}/{libtype}/{os}/{variant}/".format(**vars(args)) +prefix = "{git_sha}/{libtype}/{variant}/".format(**vars(args)) Review comment: I don't understand how this is a fix. The OS was put there for a reason apparently, so I'd first like to see a deep dive to understand the reasoning behind that parameter before removing it. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org