You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "yangzhang75 (via GitHub)" <gi...@apache.org> on 2026/06/10 01:28:51 UTC
[PR] refactor(engine): pass Python worker startup arguments by name [texera]
yangzhang75 opened a new pull request, #5597:
URL: https://github.com/apache/texera/pull/5597
<!--
Thanks for sending a pull request (PR)! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
[Contributing to Texera](https://github.com/apache/texera/blob/main/CONTRIBUTING.md)
2. Ensure you have added or run the appropriate tests for your PR
3. If the PR is work in progress, mark it a draft on GitHub.
4. Please write your PR title to summarize what this PR proposes, we
are following Conventional Commits style for PR titles as well.
5. Be sure to keep the PR description updated to reflect all changes.
-->
### What changes were proposed in this PR?
<!--
Please clarify what changes you are proposing. The purpose of this section
is to outline the changes. Here are some tips for you:
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
3. If it is a refactoring, clarify what has been changed.
3. It would be helpful to include a before-and-after comparison using
screenshots or GIFs.
4. Please consider writing useful notes for better and faster reviews.
-->
Passes Python worker startup configuration by name instead of by argv position, as proposed in the issue.
PythonWorkflowWorker (JVM) previously built ~19 positional command-line arguments, and texera_run_python_worker.py unpacked them positionally. Because the two sides agreed only by index, adding/removing/reordering one argument could silently misassign values.
- PythonWorkflowWorker.scala: build a single JSON object of named startup-config keys and pass it as one argument.
- texera_run_python_worker.py: parse that JSON and read each value by key; a missing or renamed key now raises a clear KeyError instead of silently misaligning.
The set of keys written on the Scala side and read on the Python side is identical (19 keys). No behavior change otherwise.
### Any related issues, documentation, discussions?
<!--
Please use this section to link other resources if not mentioned already.
1. If this PR fixes an issue, please include `Fixes #1234`, `Resolves #1234`
or `Closes #1234`. If it is only related, simply mention the issue number.
2. If there is design documentation, please add the link.
3. If there is a discussion in the mailing list, please add the link.
-->
Closes #5547
### How was this PR tested?
<!--
If tests were added, say they were added here. Or simply mention that if the PR
is tested with existing test cases. Make sure to include/update test cases that
check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how
you tested step by step, ideally copy and paste-able, so that other reviewers can
test and check, and descendants can verify in the future. If tests were not added,
please describe why they were not added and/or why it was difficult to add.
-->
- Verified the JSON key set written by PythonWorkflowWorker.scala exactly matches the keys read in texera_run_python_worker.py (19 keys, no drift).
- `WorkflowExecutionService/compile` (amber) succeeds.
- `scalafmtCheckAll` passes; scalafix rules (RemoveUnused, ProcedureSyntax) are satisfied (the new import is used).
- Python entry script passes `py_compile`.
- End-to-end worker launch is exercised by the existing CI integration jobs (amber-integration), which start a real Python worker.
### Was this PR authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this PR,
please include the phrase: 'Generated-by: ' followed by the name of the tool
and its version. If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Generated-by: Claude Code (Claude Opus 4.8)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4783601012
Yes, I will create one soon.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4783517158
> I found a possible Windows regression from this PR.
>
> The JVM now passes Python worker startup config as raw JSON argv, but on Windows the inner JSON quotes are stripped before Python receives it. Python gets `{workerId:w,...}` instead of valid JSON, so `json.loads(raw_config)` fails with:
>
> ```
> JSONDecodeError: Expecting property name enclosed in double quotes
> ```
Thanks. @yangzhang75 could you please create a bug issue and follow up. @carloea2 please feel free to share more context.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4783474888
I found a possible Windows regression from this PR.
The JVM now passes Python worker startup config as raw JSON argv, but on Windows the inner JSON quotes are stripped before Python receives it. Python gets `{workerId:w,...}` instead of valid JSON, so `json.loads(raw_config)` fails with:
```text
JSONDecodeError: Expecting property name enclosed in double quotes
```
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang merged PR #5597:
URL: https://github.com/apache/texera/pull/5597
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "chenlica (via GitHub)" <gi...@apache.org>.
chenlica commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4674976092
> Could you assign a reviewer for this PR? @chenlica
@Yicong-Huang can decide.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4697818734
@Yicong-Huang I extracted the config into buildStartupConfig and added Scala tests for it (all 19 keys + a round-trip).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4666496854
@Yicong-Huang I added unit tests for the mapping and missing-key behavior in the latest commit, CI's green. Could you take another look? Thanks!
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #5597:
URL: https://github.com/apache/texera/pull/5597#discussion_r3385588549
##########
amber/src/test/python/test_run_python_worker.py:
##########
@@ -0,0 +1,112 @@
+# 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 json
+from unittest import mock
+
+import pytest
+
+import texera_run_python_worker as entry
+
+
+def _full_config() -> dict:
+ """A complete startup config matching the keys PythonWorkflowWorker emits."""
+ return {
+ "workerId": "worker-1",
+ "outputPort": "5005",
+ "loggerLevel": "INFO",
+ "rPath": "",
+ "icebergCatalogType": "postgres",
+ "icebergPostgresCatalogUriWithoutScheme": "host:5432/db",
+ "icebergPostgresCatalogUsername": "pg-user",
+ "icebergPostgresCatalogPassword": "pg-pass",
+ "icebergRestCatalogUri": "",
+ "icebergRestCatalogWarehouseName": "",
+ "icebergTableNamespace": "result_ns",
+ "icebergTableStateNamespace": "state_ns",
+ "icebergFileStorageDirectoryPath": "/tmp/files",
+ "icebergTableCommitBatchSize": "100",
+ "s3Endpoint": "http://s3:9000",
+ "s3Region": "us-west-2",
+ "s3AuthUsername": "s3-user",
+ "s3AuthPassword": "s3-pass",
+ "s3LargeBinariesBaseUri": "s3://bucket/base",
+ }
+
+
+def test_main_maps_named_config_to_storage_and_worker():
+ """Each named field reaches the correct StorageConfig.initialize argument and
+ worker parameter — guarding against the silent misalignment that positional
+ argv passing allowed."""
+ config = _full_config()
+ with (
+ mock.patch.object(entry, "StorageConfig") as storage_config,
+ mock.patch.object(entry, "PythonWorker") as python_worker,
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ entry.main(json.dumps(config))
+
+ storage_config.initialize.assert_called_once_with(
+ "postgres",
+ "host:5432/db",
+ "pg-user",
+ "pg-pass",
+ "",
+ "",
+ "result_ns",
+ "state_ns",
+ "/tmp/files",
+ "100",
+ "http://s3:9000",
+ "us-west-2",
+ "s3-user",
+ "s3-pass",
+ "s3://bucket/base",
+ )
+ python_worker.assert_called_once_with(
+ worker_id="worker-1", host="localhost", output_port=5005
+ )
+ python_worker.return_value.run.assert_called_once()
+
+
+def test_main_sets_r_home_only_when_r_path_is_present(monkeypatch):
+ monkeypatch.delenv("R_HOME", raising=False)
+ config = _full_config()
+ config["rPath"] = "/opt/R"
+ with (
+ mock.patch.object(entry, "StorageConfig"),
+ mock.patch.object(entry, "PythonWorker"),
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ import os
+
+ entry.main(json.dumps(config))
+ assert os.environ["R_HOME"] == "/opt/R"
+
+
+@pytest.mark.parametrize("missing_key", sorted(_full_config().keys()))
+def test_main_raises_keyerror_when_a_field_is_missing(missing_key):
+ """A missing/renamed key fails loudly rather than being silently misassigned."""
+ config = _full_config()
+ del config[missing_key]
+ with (
+ mock.patch.object(entry, "StorageConfig"),
+ mock.patch.object(entry, "PythonWorker"),
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ with pytest.raises(KeyError):
+ entry.main(json.dumps(config))
Review Comment:
I think we need to guard it a big tighter, so that we know once a drift happens.
please include tests about
1. having extra configs. -> should fail
2. having wrong types. -> should fail
3. having wrong orders -> should succeed as json/dict is used
4. having duplicate configs -> should fail on scala side. (do we have tests for scala side yet?)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4697804848
<!-- texera-benchmarks-comment -->
## ✅ No material benchmark regressions detected
🟢 10 better · 🔴 0 worse · ⚪ 5 noise (<±5%) · 0 without baseline
> CI benchmark results are noisy; treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/27459622100)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| ⚪ | bs=10 sw=10 sl=64 | 392 | 0.239 | 24,992/39,468/39,468 us | ⚪ within ±5% / 🔴 +9.6% |
| 🟢 | bs=100 sw=10 sl=64 | 979 | 0.598 | 98,252/128,217/128,217 us | 🟢 +34.4% / 🟢 -14.5% |
| 🟢 | bs=1000 sw=10 sl=64 | 1,105 | 0.674 | 910,394/1,002,927/1,002,927 us | 🟢 +16.8% / 🟢 +9.4% |
<details><summary>Baseline details</summary>
Latest main `7ae9b35` from 2026-06-13T00:44:28.840Z
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 392 tuples/sec | 394.18 tuples/sec | 400.34 tuples/sec | -0.6% | -2.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.239 MB/s | 0.241 MB/s | 0.244 MB/s | -0.7% | -2.2% |
| bs=10 sw=10 sl=64 | p50 | 24,992 us | 24,805 us | 24,191 us | +0.8% | +3.3% |
| bs=10 sw=10 sl=64 | p95 | 39,468 us | 38,345 us | 36,018 us | +2.9% | +9.6% |
| bs=10 sw=10 sl=64 | p99 | 39,468 us | 38,345 us | 36,018 us | +2.9% | +9.6% |
| bs=100 sw=10 sl=64 | throughput | 979 tuples/sec | 729.17 tuples/sec | 867.19 tuples/sec | +34.3% | +12.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.598 MB/s | 0.445 MB/s | 0.529 MB/s | +34.4% | +13.0% |
| bs=100 sw=10 sl=64 | p50 | 98,252 us | 133,219 us | 114,874 us | -26.2% | -14.5% |
| bs=100 sw=10 sl=64 | p95 | 128,217 us | 173,178 us | 142,264 us | -26.0% | -9.9% |
| bs=100 sw=10 sl=64 | p99 | 128,217 us | 173,178 us | 142,264 us | -26.0% | -9.9% |
| bs=1000 sw=10 sl=64 | throughput | 1,105 tuples/sec | 946.3 tuples/sec | 1,010 tuples/sec | +16.8% | +9.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.674 MB/s | 0.578 MB/s | 0.616 MB/s | +16.7% | +9.4% |
| bs=1000 sw=10 sl=64 | p50 | 910,394 us | 1,059,718 us | 995,071 us | -14.1% | -8.5% |
| bs=1000 sw=10 sl=64 | p95 | 1,002,927 us | 1,104,221 us | 1,046,228 us | -9.2% | -4.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,002,927 us | 1,104,221 us | 1,046,228 us | -9.2% | -4.1% |
</details>
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,509.74,200,128000,392,0.239,24992.11,39467.96,39467.96
1,100,10,64,20,2042.43,2000,1280000,979,0.598,98252.00,128217.42,128217.42
2,1000,10,64,20,18098.78,20000,12800000,1105,0.674,910394.19,1002927.15,1002927.15
```
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4675188596
Wait I take it back. The lines in Scala are related. @yangzhang75 can you improve coverage on Scala side?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #5597:
URL: https://github.com/apache/texera/pull/5597#discussion_r3386495131
##########
amber/src/test/python/test_run_python_worker.py:
##########
@@ -0,0 +1,112 @@
+# 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 json
+from unittest import mock
+
+import pytest
+
+import texera_run_python_worker as entry
+
+
+def _full_config() -> dict:
+ """A complete startup config matching the keys PythonWorkflowWorker emits."""
+ return {
+ "workerId": "worker-1",
+ "outputPort": "5005",
+ "loggerLevel": "INFO",
+ "rPath": "",
+ "icebergCatalogType": "postgres",
+ "icebergPostgresCatalogUriWithoutScheme": "host:5432/db",
+ "icebergPostgresCatalogUsername": "pg-user",
+ "icebergPostgresCatalogPassword": "pg-pass",
+ "icebergRestCatalogUri": "",
+ "icebergRestCatalogWarehouseName": "",
+ "icebergTableNamespace": "result_ns",
+ "icebergTableStateNamespace": "state_ns",
+ "icebergFileStorageDirectoryPath": "/tmp/files",
+ "icebergTableCommitBatchSize": "100",
+ "s3Endpoint": "http://s3:9000",
+ "s3Region": "us-west-2",
+ "s3AuthUsername": "s3-user",
+ "s3AuthPassword": "s3-pass",
+ "s3LargeBinariesBaseUri": "s3://bucket/base",
+ }
+
+
+def test_main_maps_named_config_to_storage_and_worker():
+ """Each named field reaches the correct StorageConfig.initialize argument and
+ worker parameter — guarding against the silent misalignment that positional
+ argv passing allowed."""
+ config = _full_config()
+ with (
+ mock.patch.object(entry, "StorageConfig") as storage_config,
+ mock.patch.object(entry, "PythonWorker") as python_worker,
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ entry.main(json.dumps(config))
+
+ storage_config.initialize.assert_called_once_with(
+ "postgres",
+ "host:5432/db",
+ "pg-user",
+ "pg-pass",
+ "",
+ "",
+ "result_ns",
+ "state_ns",
+ "/tmp/files",
+ "100",
+ "http://s3:9000",
+ "us-west-2",
+ "s3-user",
+ "s3-pass",
+ "s3://bucket/base",
+ )
+ python_worker.assert_called_once_with(
+ worker_id="worker-1", host="localhost", output_port=5005
+ )
+ python_worker.return_value.run.assert_called_once()
+
+
+def test_main_sets_r_home_only_when_r_path_is_present(monkeypatch):
+ monkeypatch.delenv("R_HOME", raising=False)
+ config = _full_config()
+ config["rPath"] = "/opt/R"
+ with (
+ mock.patch.object(entry, "StorageConfig"),
+ mock.patch.object(entry, "PythonWorker"),
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ import os
+
+ entry.main(json.dumps(config))
+ assert os.environ["R_HOME"] == "/opt/R"
+
+
+@pytest.mark.parametrize("missing_key", sorted(_full_config().keys()))
+def test_main_raises_keyerror_when_a_field_is_missing(missing_key):
+ """A missing/renamed key fails loudly rather than being silently misassigned."""
+ config = _full_config()
+ del config[missing_key]
+ with (
+ mock.patch.object(entry, "StorageConfig"),
+ mock.patch.object(entry, "PythonWorker"),
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ with pytest.raises(KeyError):
+ entry.main(json.dumps(config))
Review Comment:
Addressed all four in the latest commit:
1. extra/unknown key → ValueError
2. non-string value → TypeError
3. key order → still maps correctly (added a test)
4. duplicate keys → fail on the Scala side via encodeStartupConfig (built from a Seq, not a Map), plus a new ScalaTest
Added Python tests for each case too. Thanks!
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4665642884
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/5597?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 52.40%. Comparing base ([`07ca5d4`](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1e6d0ca`](https://app.codecov.io/gh/apache/texera/commit/1e6d0ca30319aa41e5b6284c62e2202f2258621b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/texera/pull/5597?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [amber/src/main/python/texera\_run\_python\_worker.py](https://app.codecov.io/gh/apache/texera/pull/5597?src=pr&el=tree&filepath=amber%2Fsrc%2Fmain%2Fpython%2Ftexera_run_python_worker.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1iZXIvc3JjL21haW4vcHl0aG9uL3RleGVyYV9ydW5fcHl0aG9uX3dvcmtlci5weQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/texera/pull/5597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #5597 +/- ##
============================================
+ Coverage 52.38% 52.40% +0.02%
Complexity 2484 2484
============================================
Files 1070 1070
Lines 41359 41342 -17
Branches 4441 4436 -5
============================================
Hits 21666 21666
+ Misses 18427 18410 -17
Partials 1266 1266
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.61% <ø> (ø)` | | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.76% <ø> (ø)` | | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.37% <ø> (+0.06%)` | :arrow_up: | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `1.65% <ø> (ø)` | | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.06% <ø> (ø)` | | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.21% <ø> (ø)` | | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.91% <ø> (ø)` | | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.62% <0.00%> (-0.10%)` | :arrow_down: | |
| [python](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.80% <ø> (+0.04%)` | :arrow_up: | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.69% <ø> (ø)` | | Carriedforward from [07ca5d4](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/5597?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "xuang7 (via GitHub)" <gi...@apache.org>.
xuang7 commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4665828017
Could you assign a reviewer for this PR? @chenlica
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #5597:
URL: https://github.com/apache/texera/pull/5597#issuecomment-4675183358
> ## [Codecov](https://app.codecov.io/gh/apache/texera/pull/5597?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> :x: Patch coverage is `47.05882%` with `27 lines` in your changes missing coverage. Please review.
> :white_check_mark: Project coverage is 52.41%. Comparing base ([`07ca5d4`](https://app.codecov.io/gh/apache/texera/commit/07ca5d4cd6efebf2de284a599db1e88f2a778c08?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cb714f5`](https://app.codecov.io/gh/apache/texera/commit/cb714f527ef72d44a28d8a38f2f3fde520c5939c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :warning: Report is 1 commits behind head on main.
> :white_check_mark: All tests successful. No failed tests found.
>
> | [Files with missing lines](https://app.codecov.io/gh/apache/texera/pull/5597?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
> |---|---|---|
> | [...chitecture/pythonworker/PythonWorkflowWorker.scala](https://app.codecov.io/gh/apache/texera/pull/5597?src=pr&el=tree&filepath=amber%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Ftexera%2Famber%2Fengine%2Farchitecture%2Fpythonworker%2FPythonWorkflowWorker.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1iZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS90ZXhlcmEvYW1iZXIvZW5naW5lL2FyY2hpdGVjdHVyZS9weXRob253b3JrZXIvUHl0aG9uV29ya2Zsb3dXb3JrZXIuc2NhbGE=) | 13.33% | [25 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/texera/pull/5597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [amber/src/main/python/texera\_run\_python\_worker.py](https://app.codecov.io/gh/apache/texera/pull/5597?src=pr&el=tree&filepath=amber%2Fsrc%2Fmain%2Fpython%2Ftexera_run_python_worker.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1iZXIvc3JjL21haW4vcHl0aG9uL3RleGVyYV9ydW5fcHl0aG9uX3dvcmtlci5weQ==) | 95.23% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/texera/pull/5597?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
>
> <details><summary>Additional details and impacted files</summary>
>
>
>
> ```diff
> @@ Coverage Diff @@
> ## main #5597 +/- ##
> ============================================
> + Coverage 52.38% 52.41% +0.03%
> + Complexity 2484 2479 -5
> ============================================
> Files 1070 1070
> Lines 41359 41388 +29
> Branches 4441 4442 +1
> ============================================
> + Hits 21666 21695 +29
> + Misses 18427 18422 -5
> - Partials 1266 1271 +5
> ```
>
> | [Flag](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
> |---|---|---|---|
> | [access-control-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.61% <ø> (ø)` | | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [agent-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.76% <ø> (ø)` | | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [amber](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.26% <13.33%> (-0.05%)` | :arrow_down: | |
> | [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `1.65% <ø> (ø)` | | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [config-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.06% <ø> (ø)` | | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [file-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.21% <ø> (ø)` | | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [frontend](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.91% <ø> (ø)` | | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [pyamber](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.07% <95.23%> (+0.35%)` | :arrow_up: | |
> | [python](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.82% <ø> (+0.06%)` | :arrow_up: | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> | [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/5597/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.69% <ø> (ø)` | | Carriedforward from [72affa3](https://app.codecov.io/gh/apache/texera/commit/72affa391e4e04516aa392d2fad5b3f677fcd262?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
>
> *This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
> </details>
>
> [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/5597?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> <details><summary> :rocket: New features to boost your workflow: </summary>
>
> - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
> </details>
Lines in Scala file are uncovered, but unrelate to this change. We can cover them 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.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(engine): pass Python worker startup arguments by name [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #5597:
URL: https://github.com/apache/texera/pull/5597#discussion_r3404998867
##########
amber/src/test/python/test_run_python_worker.py:
##########
@@ -0,0 +1,112 @@
+# 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 json
+from unittest import mock
+
+import pytest
+
+import texera_run_python_worker as entry
+
+
+def _full_config() -> dict:
+ """A complete startup config matching the keys PythonWorkflowWorker emits."""
+ return {
+ "workerId": "worker-1",
+ "outputPort": "5005",
+ "loggerLevel": "INFO",
+ "rPath": "",
+ "icebergCatalogType": "postgres",
+ "icebergPostgresCatalogUriWithoutScheme": "host:5432/db",
+ "icebergPostgresCatalogUsername": "pg-user",
+ "icebergPostgresCatalogPassword": "pg-pass",
+ "icebergRestCatalogUri": "",
+ "icebergRestCatalogWarehouseName": "",
+ "icebergTableNamespace": "result_ns",
+ "icebergTableStateNamespace": "state_ns",
+ "icebergFileStorageDirectoryPath": "/tmp/files",
+ "icebergTableCommitBatchSize": "100",
+ "s3Endpoint": "http://s3:9000",
+ "s3Region": "us-west-2",
+ "s3AuthUsername": "s3-user",
+ "s3AuthPassword": "s3-pass",
+ "s3LargeBinariesBaseUri": "s3://bucket/base",
+ }
+
+
+def test_main_maps_named_config_to_storage_and_worker():
+ """Each named field reaches the correct StorageConfig.initialize argument and
+ worker parameter — guarding against the silent misalignment that positional
+ argv passing allowed."""
+ config = _full_config()
+ with (
+ mock.patch.object(entry, "StorageConfig") as storage_config,
+ mock.patch.object(entry, "PythonWorker") as python_worker,
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ entry.main(json.dumps(config))
+
+ storage_config.initialize.assert_called_once_with(
+ "postgres",
+ "host:5432/db",
+ "pg-user",
+ "pg-pass",
+ "",
+ "",
+ "result_ns",
+ "state_ns",
+ "/tmp/files",
+ "100",
+ "http://s3:9000",
+ "us-west-2",
+ "s3-user",
+ "s3-pass",
+ "s3://bucket/base",
+ )
+ python_worker.assert_called_once_with(
+ worker_id="worker-1", host="localhost", output_port=5005
+ )
+ python_worker.return_value.run.assert_called_once()
+
+
+def test_main_sets_r_home_only_when_r_path_is_present(monkeypatch):
+ monkeypatch.delenv("R_HOME", raising=False)
+ config = _full_config()
+ config["rPath"] = "/opt/R"
+ with (
+ mock.patch.object(entry, "StorageConfig"),
+ mock.patch.object(entry, "PythonWorker"),
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ import os
+
+ entry.main(json.dumps(config))
+ assert os.environ["R_HOME"] == "/opt/R"
+
+
+@pytest.mark.parametrize("missing_key", sorted(_full_config().keys()))
+def test_main_raises_keyerror_when_a_field_is_missing(missing_key):
+ """A missing/renamed key fails loudly rather than being silently misassigned."""
+ config = _full_config()
+ del config[missing_key]
+ with (
+ mock.patch.object(entry, "StorageConfig"),
+ mock.patch.object(entry, "PythonWorker"),
+ mock.patch.object(entry, "init_loguru_logger"),
+ ):
+ with pytest.raises(KeyError):
+ entry.main(json.dumps(config))
Review Comment:
thanks!
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org