Add include_samples=True #263

beccasaurus · 2019-06-13T21:51:50Z

Add support for GAPIC generated code samples.

You can find a detailed description of what include_samples=True does here:

What does include_samples=True do?

This prototype was used to send these initial PRs demonstrating generated code samples:

^--- PRs have since been updated to use include_samples=True from this branch.

This is a supported feature of GAPIC.

The official method of testing generated samples is sample-tester.

Library developers define required resources for code samples in sample_resources.yaml files.

Support for include_samples=True based on implementation of include_protos=True.

`TODO(beccasaurus)`

Add include_samples to README
Update sample manifest so you can call sample-tester from any directory
(Requires in-progress implementation of: allow running samples with the manifest file dir being the working dir sample-tester#73)
(Based on feedback from PHP)
Show example of working Java (PR to go along with this), ensure the gen-manifest is correct
Demonstrate working with pre-existing hand-written samples + generated samples

/cc @googleapis/samplegen @dwsupplee

go/library-samples (internal)

Add support for copying *.test.yaml files into samples/{version}/test/

From definitions in sample_resources.yaml files

This code didn't work because glob() returns a generator. It wasn't necessary to add a conditional around creation of the test/ directory because it's needed for the manifest file as well.

synthtool/gcp/gapic_generator.py

Expect exceptions from shell.run for sample-tester and do not blow up. Example message when sample-tester is not installed to generate manifest: (Note: sample-tester only runs when samples are present) synthtool > Failed executing sample-tester gen-manifest --bin php --env php --chdir samples --output v1p1beta1/test/samples.manifest.yaml v1p1beta1/SpeechAdaptationBeta.php v1p1beta1/SpeechTranscribeDiarizationBeta.php v1p1beta1/SpeechQuickstartBeta.php v1p1beta1/SpeechTranscribeAutoPunctuationBeta.php v1p1beta1/SpeechContextsClassesBeta.php v1p1beta1/SpeechTranscribeMultilanguageBeta.php v1p1beta1/SpeechTranscribeRecognitionMetadataBeta.php v1p1beta1/SpeechTranscribeWordLevelConfidenceBeta.php: pyenv: sample-tester: command not found synthtool > sample-tester failed to run (may not be installed)

beccasaurus · 2019-06-17T23:22:41Z

Kokoro has been stuck on: Expected — Waiting for status to be reported.
Is there anything I should do to kick it to run?

Let me know if I can change anything / provide any information to aid in review.

Here are some considerations that I made:

Follow the exact same approach as include_protos=True for consistency
Everything is put under genfiles.
Nothing pollutes the workspace where synth was invoked from!
requests is used for pulling down files for simplicity and to be idiomatic
(I tried using GCS client and didn't like it)
Fails happily if testing tool isn't available (similar to how I've seen blacken fail, for example).
Doesn't blow up, warning only.

The method is just barely shorter than the _generate_code method.
If it should be extracted to a class, lemme know. Available to make any requested changes!

busunkim96

🎆

busunkim96 · 2019-06-18T21:44:18Z

@beccasaurus LGTM! I think our linter is unhappy, but a quick nox -s blacken should fix that. :)

…to include_samples

For code samples, we need to know the version number of the API. Samples are output into directories like: sample/{version}/ Most languages use lowercase version numbers, e.g. v1 or v1p1beta1 Other languages (PHP) capitalize the 'v', e.g. V1 or V1p1beta1 This checks for both. Note: this update does not support full case insensitivity, only allows for the 'v' to be either lower or uppercase for now.

Remove this code now that the following issue was resolved: googleapis/sample-tester#70

The `gen-manifest` command is still a part of the sample-tester PyPi package. Note: manifest generation will eventually move into the generators themselves. For the time being, `gen-manifest` is the recommended tool.

For now. Start with Python, Ruby, Node.js, PHP. Follow-up later to add support for the remaining languages: Java, C#. Go does not use SynthTool at this time.

Now that the sample-tester issue[0] has been resolved, the way key/value pairs are passed to gen-manifest has been updated to be far more flexible. You can read more at: https://sample-tester.readthedocs.io/ [0]: googleapis/sample-tester#70

Reminder: this is intentionally only supporting Python, Ruby, PHP, and Node.js More language support to come in the future

beccasaurus · 2019-08-21T22:18:42Z

~ Just pushed a number of changes ~

This is production ready for: 🐍 Python, 🐘 PHP, 💎 Ruby, and 🚀 Node.js

This is ready for re-review 🔍

Note: Support for ☕️ Java and #️⃣ C# explicitly de-scoped (will add later)

☑️ [Response to Feedback]

Since the PR was first sent, there have been a number of changes made
to the sample generation tooling as a response to user and reviewer feedback:

sample-tester can be called from any directory
- This was a response to feedback from the PHP samplegen PR
- This makes it simpler to run sample tests from CI (e.g. Kokoro, CircleCI, Travis, etc)
- Before this, sample-tester needed to be run from certain directories. Now it knows the location of sample files relative to the project root and can be invoked from anywhere.
sample-tester no longer requires passing individual test files
- you can simply run $ sample-tester or $ sample-tester samples/
- this should be more familiar to developers accustomed to other test runners
  (e.g mocha/ava, pytest, junit, phpunit, rspec/minitest, go test, etc etc)
^--- changes relevant to integrating samples into client library repositories

ℹ️ [Misc]

There have been a number of other, unrelated improvements to the tooling:

samples can now be authored in their own, standalone .yaml files
- no longer required to configure samples in _gapic.yaml configs (which is being deprecated)
- samples are authored in a new, improved, easier to read and write .yaml format
  
  note: the samples in googleapis/googleapis do not yet reflect the new format
all YAML files now have a type: and schema version for tooling production readiness
- using this for backwards compatible support of old sample/test/manifest formats
Java packaging improvements
C# is now supported

Java + C# support will be added in the future (out of scope for this PR)

/cc @googleapis/samplegen 🦇

beccasaurus · 2019-08-21T22:57:04Z

@busunkim96 Kokoro is happy, but FYI: I tweaked the code a bit since the last review, would love another look-see 👀 Nothing more to add on my end, it's in a happy state 🍰

beccasaurus · 2019-08-22T17:44:46Z

Ty kindly!

Rebecca Taylor added 8 commits June 12, 2019 12:00

Add include_samples

fad626b

Add support for copying *.test.yaml files into samples/{version}/test/

Download samples resources from Cloud Storage

d881f06

From definitions in sample_resources.yaml files

Download sample resources using http(s)://

175faf4

Tweak: move _include_samples in file

8094a32

Finished, working include_samples=True

8f12f6d

Code cleanup [include_samples]

85866fd

Always make samples/{version}/test/

2711231

This code didn't work because glob() returns a generator. It wasn't necessary to add a conditional around creation of the test/ directory because it's needed for the manifest file as well.

Revert change to setup.py

fda2887

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 13, 2019

beccasaurus mentioned this pull request Jun 13, 2019

Add Speech generated samples googleapis/google-cloud-php#1991

Closed

4 tasks

busunkim96 reviewed Jun 13, 2019

View reviewed changes

synthtool/gcp/gapic_generator.py Outdated Show resolved Hide resolved

Rebecca Taylor added 9 commits June 13, 2019 16:22

Move imports to top of file

44ef908

Merge branch 'master' into include_samples

cd64d5b

Re-order imports

07e2a9e

Fix comment (*.py only refers to Python samples)

999720d

Clean up docstring to be even more clear :)

3743107

De-dupe reference to samples/test directory

42c22d6

s/system test/sample test/g

54cf023

Fix Java sample-tester invocation

989a5a6

bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 17, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 17, 2019

busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 18, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 18, 2019

Merge branch 'master' into include_samples

675151d

busunkim96 approved these changes Jun 18, 2019

View reviewed changes

yoshi-automation added the 🚨 This issue needs some love. label Jun 20, 2019

Merge branch 'master' into include_samples

0406039

yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019

Rebecca Taylor added 11 commits July 26, 2019 09:50

Merge branch 'include_samples' of github.com:beccasaurus/synthtool in…

77c7182

…to include_samples

Merge branch 'master' into include_samples

12da566

Remove previously needed sample hack

a651639

Remove this code now that the following issue was resolved: googleapis/sample-tester#70

Manifests now generated with $ gen-manifest

68ab891

The `gen-manifest` command is still a part of the sample-tester PyPi package. Note: manifest generation will eventually move into the generators themselves. For the time being, `gen-manifest` is the recommended tool.

Remove Java samplegen (keep Python, Ruby, Node.js, PHP)

045eac8

For now. Start with Python, Ruby, Node.js, PHP. Follow-up later to add support for the remaining languages: Java, C#. Go does not use SynthTool at this time.

Update message for $ gen-manifest command

d7d3118

Simplify comment

0ac897d

Update gen-manifest CLI arguments

96e40f1

Now that the sample-tester issue[0] has been resolved, the way key/value pairs are passed to gen-manifest has been updated to be far more flexible. You can read more at: https://sample-tester.readthedocs.io/ [0]: googleapis/sample-tester#70

More comments documenting samples

882dadb

Cleanup $ gen-manifest code

980d96f

Reminder: this is intentionally only supporting Python, Ruby, PHP, and Node.js More language support to come in the future

Rebecca Taylor added 4 commits August 21, 2019 15:21

nox -s blacken

1c49a80

Trigger linter again (added training newline)

15719a0

Re-run nox -s blacken

7c4e94f

Remove unused local variable exp

564d9d9

busunkim96 approved these changes Aug 22, 2019

View reviewed changes

busunkim96 merged commit e4aa9bb into googleapis:master Aug 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add include_samples=True #263

Add include_samples=True #263

beccasaurus commented Jun 13, 2019 •

edited

Loading

beccasaurus commented Jun 17, 2019

busunkim96 left a comment

busunkim96 commented Jun 18, 2019

beccasaurus commented Aug 21, 2019

beccasaurus commented Aug 21, 2019 •

edited

Loading

beccasaurus commented Aug 22, 2019

Add include_samples=True #263

Add include_samples=True #263

Conversation

beccasaurus commented Jun 13, 2019 • edited Loading

TODO(beccasaurus)

beccasaurus commented Jun 17, 2019

busunkim96 left a comment

Choose a reason for hiding this comment

busunkim96 commented Jun 18, 2019

beccasaurus commented Aug 21, 2019

beccasaurus commented Aug 21, 2019 • edited Loading

beccasaurus commented Aug 22, 2019

beccasaurus commented Jun 13, 2019 •

edited

Loading

`TODO(beccasaurus)`

beccasaurus commented Aug 21, 2019 •

edited

Loading