tvalentyn commented on code in PR #38688:
URL: https://github.com/apache/beam/pull/38688#discussion_r3306218534
##########
sdks/python/apache_beam/runners/portability/stager.py:
##########
@@ -781,23 +787,60 @@ def _populate_requirements_cache(
]
if populate_cache_with_sdists:
- cmd_args.extend(['--no-binary', ':all:'])
+ download_dir = tempfile.mkdtemp(dir=temp_directory)
+ cmd_args.extend(['--dest', download_dir, '--no-binary', ':all:'])
+ _LOGGER.info('Executing command: %s', cmd_args)
+ processes.check_output(cmd_args, stderr=processes.STDOUT)
else:
language_implementation_tag = 'cp'
abi_suffix = 'm' if sys.version_info < (3, 8) else ''
abi_tag = 'cp%d%d%s' % (
sys.version_info[0], sys.version_info[1], abi_suffix)
- platform_tag = Stager._get_platform_for_default_sdk_container()
- cmd_args.extend([
- '--implementation',
- language_implementation_tag,
- '--abi',
- abi_tag,
- '--platform',
- platform_tag
- ])
- _LOGGER.info('Executing command: %s', cmd_args)
- processes.check_output(cmd_args, stderr=processes.STDOUT)
+ preferred_platform = Stager._get_platform_for_default_sdk_container()
Review Comment:
high-level comments:
- I'd prefer to extract this logic into a helper since this method is
getting long. Options (no strong opinion):
_get_platform_for_default_sdk_container(package_name) ,
download_dist_for_package(package_name), etc.
- _get_platform_for_default_sdk_container is becoming obsolete since it no
longer fully captures the logic selecting the platform given the list we are
introducting; we should keep that logic in one place, perhaps by getting rid of
_get_platform_for_default_sdk_container and add the default into the map itself.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
