[GitHub] [beam] ihji commented on a change in pull request #10621: [BEAM-9056] Staging artifacts from environment

2020-03-05 Thread GitBox 
ihji commented on a change in pull request #10621: [BEAM-9056] Staging 
artifacts from environment
URL: https://github.com/apache/beam/pull/10621#discussion_r388509693
 
 

 ##
 File path: 
runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/SdkComponents.java
 ##
 @@ -261,14 +263,20 @@ public String registerCoder(Coder coder) throws 
IOException {
* return the same unique ID.
*/
   public String registerEnvironment(Environment env) {
+String environmentId;
 String existing = environmentIds.get(env);
 if (existing != null) {
-  return existing;
+  environmentId = existing;
+} else {
+  String name = uniqify(env.getUrn(), environmentIds.values());
+  environmentIds.put(env, name);
+  componentsBuilder.putEnvironments(name, env);
+  environmentId = name;
 }
-String name = uniqify(env.getUrn(), environmentIds.values());
-environmentIds.put(env, name);
-componentsBuilder.putEnvironments(name, env);
-return name;
+if (defaultEnvironmentId == null) {
 
 Review comment:
   If we change the signature of `registerEnvironment`, a number of test files 
(*TranslationTest, *RunnerTest) also need to be touched. I think it will create 
unnecessary noise in this PR.


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


With regards,
Apache Git Services

[GitHub] [beam] ihji commented on a change in pull request #10621: [BEAM-9056] Staging artifacts from environment

2020-03-02 Thread GitBox 
ihji commented on a change in pull request #10621: [BEAM-9056] Staging 
artifacts from environment
URL: https://github.com/apache/beam/pull/10621#discussion_r386643699
 
 

 ##
 File path: 
runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/SdkComponents.java
 ##
 @@ -261,14 +263,20 @@ public String registerCoder(Coder coder) throws 
IOException {
* return the same unique ID.
*/
   public String registerEnvironment(Environment env) {
+String environmentId;
 String existing = environmentIds.get(env);
 if (existing != null) {
-  return existing;
+  environmentId = existing;
+} else {
+  String name = uniqify(env.getUrn(), environmentIds.values());
+  environmentIds.put(env, name);
+  componentsBuilder.putEnvironments(name, env);
+  environmentId = name;
 }
-String name = uniqify(env.getUrn(), environmentIds.values());
-environmentIds.put(env, name);
-componentsBuilder.putEnvironments(name, env);
-return name;
+if (defaultEnvironmentId == null) {
 
 Review comment:
   Created a separate ticket: https://issues.apache.org/jira/browse/BEAM-9425


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


With regards,
Apache Git Services

[GitHub] [beam] ihji commented on a change in pull request #10621: [BEAM-9056] Staging artifacts from environment

2020-02-27 Thread GitBox 
ihji commented on a change in pull request #10621: [BEAM-9056] Staging 
artifacts from environment
URL: https://github.com/apache/beam/pull/10621#discussion_r385469800
 
 

 ##
 File path: sdks/python/apache_beam/runners/portability/stager.py
 ##
 @@ -331,22 +389,23 @@ def _download_file(from_url, to_path):
   def _is_remote_path(path):
 return path.find('://') != -1
 
-  def _stage_jar_packages(self, jar_packages, staging_location, temp_dir):
-# type: (...) -> List[str]
+  @staticmethod
+  def _stage_jar_packages(jar_packages, temp_dir):
 
 Review comment:
   done.


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


With regards,
Apache Git Services

[GitHub] [beam] ihji commented on a change in pull request #10621: [BEAM-9056] Staging artifacts from environment

2020-02-27 Thread GitBox 
ihji commented on a change in pull request #10621: [BEAM-9056] Staging 
artifacts from environment
URL: https://github.com/apache/beam/pull/10621#discussion_r385469821
 
 

 ##
 File path: sdks/python/apache_beam/runners/portability/stager.py
 ##
 @@ -377,34 +436,33 @@ def _stage_jar_packages(self, jar_packages, 
staging_location, temp_dir):
 
 for package in local_packages:
   basename = os.path.basename(package)
-  staged_path = FileSystems.join(staging_location, basename)
-  self.stage_artifact(package, staged_path)
-  resources.append(basename)
+  resources.append((package, basename))
 
 return resources
 
-  def _stage_extra_packages(self, extra_packages, staging_location, temp_dir):
-# type: (...) -> List[str]
+  @staticmethod
+  def _stage_extra_packages(extra_packages, temp_dir):
 
 Review comment:
   done.


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


With regards,
Apache Git Services

[GitHub] [beam] ihji commented on a change in pull request #10621: [BEAM-9056] Staging artifacts from environment

2020-02-27 Thread GitBox 
ihji commented on a change in pull request #10621: [BEAM-9056] Staging 
artifacts from environment
URL: https://github.com/apache/beam/pull/10621#discussion_r385469668
 
 

 ##
 File path: 
runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/SdkComponents.java
 ##
 @@ -88,11 +88,13 @@ public static SdkComponents create(RunnerApi.Components 
components) {
   public static SdkComponents create(PipelineOptions options) {
 SdkComponents sdkComponents = new 
SdkComponents(RunnerApi.Components.getDefaultInstance(), "");
 PortablePipelineOptions portablePipelineOptions = 
options.as(PortablePipelineOptions.class);
-sdkComponents.defaultEnvironmentId =
-sdkComponents.registerEnvironment(
-Environments.createOrGetDefaultEnvironment(
+sdkComponents.registerEnvironment(
+Environments.createOrGetDefaultEnvironment(
 portablePipelineOptions.getDefaultEnvironmentType(),
-portablePipelineOptions.getDefaultEnvironmentConfig()));
+portablePipelineOptions.getDefaultEnvironmentConfig())
+.toBuilder()
+.addAllDependencies(Environments.getArtifacts(options))
 
 Review comment:
   agreed. moved to createOrGetDefaultEnvironment.


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


With regards,
Apache Git Services

[GitHub] [beam] ihji commented on a change in pull request #10621: [BEAM-9056] Staging artifacts from environment

2020-02-27 Thread GitBox 
ihji commented on a change in pull request #10621: [BEAM-9056] Staging 
artifacts from environment
URL: https://github.com/apache/beam/pull/10621#discussion_r385469838
 
 

 ##
 File path: sdks/python/apache_beam/runners/portability/stager.py
 ##
 @@ -547,21 +601,22 @@ def 
_desired_sdk_filename_in_staging_location(sdk_location):
 else:
   return DATAFLOW_SDK_TARBALL_FILE
 
-  def _stage_beam_sdk(self, sdk_remote_location, staging_location, temp_dir):
-# type: (...) -> List[str]
+  @staticmethod
+  def _stage_beam_sdk(sdk_remote_location, temp_dir):
 
 Review comment:
   done.


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


With regards,
Apache Git Services

[GitHub] [beam] ihji commented on a change in pull request #10621: [BEAM-9056] Staging artifacts from environment

2020-02-27 Thread GitBox 
ihji commented on a change in pull request #10621: [BEAM-9056] Staging 
artifacts from environment
URL: https://github.com/apache/beam/pull/10621#discussion_r385469751
 
 

 ##
 File path: 
runners/portability/java/src/main/java/org/apache/beam/runners/portability/PortableRunner.java
 ##
 @@ -200,11 +143,44 @@ public PipelineResult run(Pipeline pipeline) {
   prepareJobResponse.getArtifactStagingEndpoint();
   String stagingSessionToken = prepareJobResponse.getStagingSessionToken();
 
+  ImmutableList.Builder filesToStageBuilder = 
ImmutableList.builder();
 
 Review comment:
   done.


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


With regards,
Apache Git Services