You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "dermasmid (via GitHub)" <gi...@apache.org> on 2024/01/07 02:34:26 UTC
[PR] [Typescript] no serialization Pardo [beam]
dermasmid opened a new pull request, #29944:
URL: https://github.com/apache/beam/pull/29944
Typescript Dofn's are very limited due to limited support for serialization in Js as a whole.
The following PR gets around serialization entirely by, importing the users function from their code, which allows for basically any code to run.
There's a few ways to implement it, none of them seem to be perfect, but ive done two here:
1. import the function by using require on the user's module
2. having the user register the `fn` and save it in the `global` object
The drawbacks:
1. it's not always possible to determine when the users `fn` can be imported from automatically
2. it's not as clean, we need to require() the users module to get those registers to run
Both approaches will result in the users pipeline code to run on the worker, which means we need to avoid starting the pipeline in case we run on the worker (similar to python's __name__ == "__main__"), but we can prob detect it in the sdk code so the user does not have to deal with it.
The reason this was so easy to implement is because we already pack in then install the user's code on the workers, so the only thing we need to solve is how to import the fn that that user passed in while creating the pipeline.
In any case this PR is meant to showcase the idea and start a conversation
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "dermasmid (via GitHub)" <gi...@apache.org>.
dermasmid commented on code in PR #29944:
URL: https://github.com/apache/beam/pull/29944#discussion_r1451597281
##########
sdks/typescript/test/localParDo.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { registerLocalParDo } from "../src/apache_beam/serialization";
+import * as beam from "../src/apache_beam";
+import { directRunner } from "../src/apache_beam/runners/direct_runner";
+
+export const log = {
+ exportName: 'log',
+ process: function (lines) {
+ console.log(lines);
+ return lines
+ }
+};
+
+registerLocalParDo(log)
Review Comment:
in worker_main L64
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #29944:
URL: https://github.com/apache/beam/pull/29944#discussion_r1451043565
##########
sdks/typescript/src/apache_beam/transforms/localParDo.ts:
##########
@@ -0,0 +1,285 @@
+/*
Review Comment:
It looks like this is a copy of parDo, but it's hard to see the diff.
##########
sdks/typescript/src/apache_beam/runners/runner.ts:
##########
@@ -64,21 +64,22 @@ export class PipelineResult {
* transforms, non-trivial windowing, etc. are used).
*/
export function createRunner(options: any = {}): Runner {
- let runnerConstructor: (any) => Runner;
if (options.runner === undefined || options.runner === "default") {
- runnerConstructor = defaultRunner;
- } else if (options.runner === "direct") {
- runnerConstructor = require("./direct_runner").directRunner;
- } else if (options.runner === "universal") {
- runnerConstructor = require("./universal").universalRunner;
- } else if (options.runner === "flink") {
- runnerConstructor = require("./flink").flinkRunner;
- } else if (options.runner === "dataflow") {
- runnerConstructor = require("./dataflow").dataflowRunner;
- } else {
- throw new Error("Unknown runner: " + options.runner);
+ return defaultRunner(options);
}
- return runnerConstructor(options);
+ if (options.runner === "direct") {
Review Comment:
I'm fine with returning directly, but lets keep the elif statements to highlight the mutual exclusivity.
##########
sdks/typescript/test/localParDo.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { registerLocalParDo } from "../src/apache_beam/serialization";
+import * as beam from "../src/apache_beam";
+import { directRunner } from "../src/apache_beam/runners/direct_runner";
+
+export const log = {
+ exportName: 'log',
Review Comment:
I assume these must be unique?
##########
sdks/typescript/test/localParDo.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { registerLocalParDo } from "../src/apache_beam/serialization";
+import * as beam from "../src/apache_beam";
+import { directRunner } from "../src/apache_beam/runners/direct_runner";
+
+export const log = {
+ exportName: 'log',
+ process: function (lines) {
+ console.log(lines);
+ return lines
+ }
+};
+
+registerLocalParDo(log)
Review Comment:
I'm not quite following how we ensure this gets called.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29944:
URL: https://github.com/apache/beam/pull/29944#issuecomment-2016796555
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #29944: [Typescript] no serialization Pardo
URL: https://github.com/apache/beam/pull/29944
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "dermasmid (via GitHub)" <gi...@apache.org>.
dermasmid commented on code in PR #29944:
URL: https://github.com/apache/beam/pull/29944#discussion_r1451745109
##########
sdks/typescript/src/apache_beam/transforms/localParDo.ts:
##########
@@ -0,0 +1,285 @@
+/*
Review Comment:
im not sure its a good idea to copy the file, instead i think we should extend Pardo.ts to support both.
the question is if we even want both.... this entire PR is more of a proposal, i wanted to get more input on this idea before really implementing it cleanly
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "dermasmid (via GitHub)" <gi...@apache.org>.
dermasmid commented on code in PR #29944:
URL: https://github.com/apache/beam/pull/29944#discussion_r1451597301
##########
sdks/typescript/test/localParDo.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { registerLocalParDo } from "../src/apache_beam/serialization";
+import * as beam from "../src/apache_beam";
+import { directRunner } from "../src/apache_beam/runners/direct_runner";
+
+export const log = {
+ exportName: 'log',
Review Comment:
yes
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29944:
URL: https://github.com/apache/beam/pull/29944#issuecomment-2002447790
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #29944:
URL: https://github.com/apache/beam/pull/29944#discussion_r1454172313
##########
sdks/typescript/test/localParDo.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { registerLocalParDo } from "../src/apache_beam/serialization";
+import * as beam from "../src/apache_beam";
+import { directRunner } from "../src/apache_beam/runners/direct_runner";
+
+export const log = {
+ exportName: 'log',
Review Comment:
Across what domain? If it's globally, perhaps we should establish (if just by example) some kind of a name-spacing convention? (This is especially true when users want to invoke libraries and my not be aware of what DoFns, with their names, they may hold.)
##########
sdks/typescript/src/apache_beam/transforms/localParDo.ts:
##########
@@ -0,0 +1,285 @@
+/*
Review Comment:
+1. I was thinking it's just hard to see what's new with the file being copied rather than updated.
##########
sdks/typescript/src/apache_beam/runners/runner.ts:
##########
@@ -64,21 +64,22 @@ export class PipelineResult {
* transforms, non-trivial windowing, etc. are used).
*/
export function createRunner(options: any = {}): Runner {
- let runnerConstructor: (any) => Runner;
if (options.runner === undefined || options.runner === "default") {
- runnerConstructor = defaultRunner;
- } else if (options.runner === "direct") {
- runnerConstructor = require("./direct_runner").directRunner;
- } else if (options.runner === "universal") {
- runnerConstructor = require("./universal").universalRunner;
- } else if (options.runner === "flink") {
- runnerConstructor = require("./flink").flinkRunner;
- } else if (options.runner === "dataflow") {
- runnerConstructor = require("./dataflow").dataflowRunner;
- } else {
- throw new Error("Unknown runner: " + options.runner);
+ return defaultRunner(options);
}
- return runnerConstructor(options);
+ if (options.runner === "direct") {
Review Comment:
Probably just a matter of preference. I'm happy with whatever is most idiomatic for typescript, and am happy to defer to you on that.
##########
sdks/typescript/test/localParDo.ts:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { registerLocalParDo } from "../src/apache_beam/serialization";
+import * as beam from "../src/apache_beam";
+import { directRunner } from "../src/apache_beam/runners/direct_runner";
+
+export const log = {
+ exportName: 'log',
+ process: function (lines) {
+ console.log(lines);
+ return lines
+ }
+};
+
+registerLocalParDo(log)
Review Comment:
OK, so this is just for DoFns that get called in the main (aka `process.argv[1]`) file? We'd still need serialization capabilities for functions defined elsewhere?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "dermasmid (via GitHub)" <gi...@apache.org>.
dermasmid commented on code in PR #29944:
URL: https://github.com/apache/beam/pull/29944#discussion_r1451597194
##########
sdks/typescript/src/apache_beam/runners/runner.ts:
##########
@@ -64,21 +64,22 @@ export class PipelineResult {
* transforms, non-trivial windowing, etc. are used).
*/
export function createRunner(options: any = {}): Runner {
- let runnerConstructor: (any) => Runner;
if (options.runner === undefined || options.runner === "default") {
- runnerConstructor = defaultRunner;
- } else if (options.runner === "direct") {
- runnerConstructor = require("./direct_runner").directRunner;
- } else if (options.runner === "universal") {
- runnerConstructor = require("./universal").universalRunner;
- } else if (options.runner === "flink") {
- runnerConstructor = require("./flink").flinkRunner;
- } else if (options.runner === "dataflow") {
- runnerConstructor = require("./dataflow").dataflowRunner;
- } else {
- throw new Error("Unknown runner: " + options.runner);
+ return defaultRunner(options);
}
- return runnerConstructor(options);
+ if (options.runner === "direct") {
Review Comment:
it thought it will be more readable this way, but having both the else if, and the return would be even less readable.
the "early return" is a common pattern in JS, but i dont feel strongly about it, and can revert if you want
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Typescript] no serialization Pardo [beam]
Posted by "dermasmid (via GitHub)" <gi...@apache.org>.
dermasmid commented on PR #29944:
URL: https://github.com/apache/beam/pull/29944#issuecomment-1890757411
> It would be really good to make it easy to clean up serialization.
can you explain what you mean?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org