Skip to content

Add support for FLOAT to Python RowCoder - #22626

Merged
TheNeuralBit merged 2 commits into
apache:masterfrom
TheNeuralBit:python-float
Aug 12, 2022
Merged

Add support for FLOAT to Python RowCoder#22626
TheNeuralBit merged 2 commits into
apache:masterfrom
TheNeuralBit:python-float

Conversation

@TheNeuralBit

Copy link
Copy Markdown
Member

Part of #19815

This adds support for encoding the schema type FLOAT to Python's RowCoder. This is added in a new coder, SinglePrecisionFloatCoder, designed to be compatible with Java's FloatCoder.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

@lostluck it looks like Go might be using the same coder for single-precision floats as for double-precision, is that right? From the Go PreCommit:

13:58:13 --- FAIL: TestStandardCoders (0.03s)
13:58:13 --- FAIL: TestStandardCoders/beam:coder:row:v1#06 (0.00s)
13:58:13 fromyaml_test.go:42: Failed "{beam:coder:row:v1 13:58:13 13:58:13 �f_float�����$8c97b6c5-69e5-4733-907b-26cd8edae612 [] false}": err decoding "\x01\x00\x00\x00\x00\x00": decoding a struct { F_float float32 "beam:\"f_float\"" }
13:58:13 caused by:
13:58:13 error decoding double field
13:58:13 caused by:
13:58:13 EOF
13:58:13 FAIL

@TheNeuralBit

TheNeuralBit commented Aug 8, 2022

Copy link
Copy Markdown
MemberAuthor

Yes that does seem to be the case:

casereflect.Float32, reflect.Float64:
returntypeEncoderFieldReflect{encode: func(rv reflect.Value, w io.Writer) error {
returnEncodeDouble(rv.Float(), w)
}}, nil

Would you be opposed to me sending a change to encode as single-precision in that case?

@codecov

codecovBot commented Aug 8, 2022

Copy link
Copy Markdown

Codecov Report

Merging #22626 (bcb3822) into master (7a9bb76) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

@@ Coverage Diff @@## master #22626 +/- ##
==========================================
- Coverage 74.19% 74.19% -0.01% 
==========================================
Files 709 709 Lines 93499 93524 +25 ==========================================
+ Hits 69370 69387 +17 - Misses 22852 22860 +8 
Partials 1277 1277 
FlagCoverage Δ
python83.59% <80.00%> (-0.01%)⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted FilesCoverage Δ
...ks/python/apache_beam/coders/coders_test_common.py98.54% <ø> (ø)
sdks/python/apache_beam/coders/coders.py88.00% <63.63%> (-0.35%)⬇️
sdks/python/apache_beam/coders/coder_impl.py93.91% <85.71%> (-0.06%)⬇️
sdks/python/apache_beam/coders/row_coder.py94.64% <100.00%> (+0.14%)⬆️
sdks/python/apache_beam/coders/slow_stream.py94.69% <100.00%> (+0.19%)⬆️
sdks/python/apache_beam/internal/metrics/metric.py93.00% <0.00%> (-1.00%)⬇️
...hon/apache_beam/runners/direct/test_stream_impl.py93.28% <0.00%> (-0.75%)⬇️
...eam/runners/portability/fn_api_runner/execution.py92.44% <0.00%> (-0.65%)⬇️
...ks/python/apache_beam/runners/worker/sdk_worker.py88.94% <0.00%> (-0.16%)⬇️
...on/apache_beam/runners/dataflow/dataflow_runner.py82.87% <0.00%> (-0.14%)⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @AnandInguva for label python.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Well I tried to modify Go's RowCoder but couldn't manage to get the test working right. Instead I've just skipped this case, and filed #22629 to track.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run Java PreCommit

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

@AnandInguva do you have time to review this?

@AnandInguva

Copy link
Copy Markdown
Contributor

@TheNeuralBit I am on vacation. I will be back on Aug 22nd.

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

assign to next reviewer

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

R: @tvalentyn

@github-actions

Copy link
Copy Markdown
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Oops Valentyn is also out

R: @ryanthompson591

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting there's a race with #22664. Whichever is merged later should remove this exception.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrmccluskey this test case is still failing in Go. It looks like it may just be a test infra issue, but I'm not able to debug on my own. Could you take a look?

I'm re-opening #22629 to track

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share the output from the test failure?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have shared it to begin with but it had scrolled out of my shell history and I was lazy 😬

I dropped the log in #22629 (comment)

@TheNeuralBit

Copy link
Copy Markdown
MemberAuthor

Run Go PreCommit


coder:
urn: "beam:coder:row:v1"
# f_float: float32

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why f_float instead of float or float32?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f_float here is the name of a field in the schema. We could call it "float" or "float32" instead, but it's not that important. I just followed the f_ convention used elsewhere.

---

coder:
urn: "beam:coder:row:v1"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you double check this is not a typo. For example with double this is:
beam:coder:double:v1

I would suspect this should be
beam:coder:float:v1

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reference to row coder:

Row Coder is a complex coder that is parameterized by a Beam Schema. It encodes different types of values by deferring to other coders, like beam:coder:double:v1 for DOUBLE

@TheNeuralBit
TheNeuralBit merged commit 63ba9c7 into apache:masterAug 12, 2022
MarcoRob pushed a commit to MarcoRob/beam that referenced this pull request Sep 5, 2022
* Add support for FLOAT to Python RowCoder
* Skip the new test case in Go
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@TheNeuralBit@AnandInguva@ryanthompson591@jrmccluskey