Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation revamp #4871

Merged
merged 14 commits into from
Jun 21, 2023
Merged

Documentation revamp #4871

merged 14 commits into from
Jun 21, 2023

Conversation

kellen
Copy link
Contributor

@kellen kellen commented Jun 9, 2023

Documents a bunch of previously undocumented or only-scaladoced bits of scio functionality for the site. Also documents some fundamental features in a structured way.

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #4871 (e619f72) into main (1f32fd6) will increase coverage by 0.06%.
The diff coverage is 87.02%.

❗ Current head e619f72 differs from pull request most recent head 18649c5. Consider uploading reports for the commit 18649c5 to get more accurate results

@@            Coverage Diff             @@
##             main    #4871      +/-   ##
==========================================
+ Coverage   62.89%   62.96%   +0.06%     
==========================================
  Files         282      282              
  Lines       10574    10641      +67     
  Branches      773      779       +6     
==========================================
+ Hits         6651     6700      +49     
- Misses       3923     3941      +18     
Impacted Files Coverage Δ
...fy/scio/coders/instances/kryo/AvroSerializer.scala 100.00% <ø> (ø)
...com/spotify/scio/coders/instances/AvroCoders.scala 84.21% <ø> (-0.41%) ⬇️
...-core/src/main/scala/com/spotify/scio/io/Tap.scala 79.31% <ø> (ø)
...ify/scio/io/dynamic/syntax/SCollectionSyntax.scala 90.38% <ø> (ø)
...spotify/scio/schemas/instances/AvroInstances.scala 0.00% <ø> (ø)
...scio/transforms/syntax/SCollectionSafeSyntax.scala 100.00% <ø> (ø)
...y/scio/values/PairSkewedSCollectionFunctions.scala 99.18% <ø> (ø)
...in/scala/com/spotify/scio/values/SCollection.scala 94.36% <ø> (ø)
...y/scio/extra/sorter/syntax/SCollectionSyntax.scala 94.44% <ø> (ø)
...ify/scio/jdbc/sharded/JdbcShardedReadOptions.scala 0.00% <ø> (ø)
... and 13 more

... and 4 files with indirect coverage changes

site/src/main/paradox/Builtin.md Outdated Show resolved Hide resolved
site/src/main/paradox/Builtin.md Outdated Show resolved Hide resolved
site/src/main/paradox/Builtin.md Outdated Show resolved Hide resolved
site/src/main/paradox/Builtin.md Show resolved Hide resolved
site/src/main/paradox/Builtin.md Outdated Show resolved Hide resolved
site/src/main/paradox/Builtin.md Outdated Show resolved Hide resolved
site/src/main/paradox/Builtin.md Outdated Show resolved Hide resolved
site/src/main/paradox/Scio-Unit-Tests.md Outdated Show resolved Hide resolved
@kellen kellen marked this pull request as ready for review June 13, 2023 12:48
@@ -57,7 +57,7 @@ final class PredictSCollectionOps[T](private val self: SCollection[T]) {
* @param signatureName
* name of [[org.tensorflow.framework.SignatureDef]] s to be used to run the prediction.
*/
def predict[V: Coder, W](
def predict[V: Coder](
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change. would you like to mark this one for 0.13 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah let's. The parameter is unused so not sure why it lived on, maybe for backwards compatibility?

Copy link
Contributor

@RustedBones RustedBones left a comment

Choose a reason for hiding this comment

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

That's going to be a great help for all scio-users!

site/src/main/paradox/Joins.md Outdated Show resolved Hide resolved
@@ -0,0 +1,102 @@
# Side Inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add a section to discourage map on side input as the operation is a view and will be applied on every element accessing the side input ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that seems like a better fit for the scaladocs, or a warning when using the map function.

Do we need to revisit the related PRs? It looks like the intent was to cache values to reduce any lookups into the view?

#2287 and the issues #1967 and #2101

@@ -0,0 +1,41 @@
# AsyncDoFn
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you ❤️

site/src/main/paradox/io/Tensorflow.md Outdated Show resolved Hide resolved
@kellen kellen added this to the 0.13.0 milestone Jun 15, 2023
site/src/main/paradox/extras/Annoy.md Outdated Show resolved Hide resolved
site/src/main/paradox/extras/AsyncDoFn.md Show resolved Hide resolved
site/src/main/paradox/extras/AsyncDoFn.md Outdated Show resolved Hide resolved
site/src/main/paradox/extras/AsyncDoFn.md Outdated Show resolved Hide resolved
site/src/main/paradox/extras/BigQueryAvro.md Outdated Show resolved Hide resolved
site/src/main/paradox/io/Jdbc.md Outdated Show resolved Hide resolved
@RustedBones RustedBones merged commit 2093b09 into main Jun 21, 2023
@RustedBones RustedBones deleted the kellen/docs branch June 21, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants