Skip to content

[MAJOR RELEASE] Auto-generated code and other changes#286

Merged
ehdsouza merged 40 commits intomasterfrom
v1.0.0
Nov 13, 2017
Merged

[MAJOR RELEASE] Auto-generated code and other changes#286
ehdsouza merged 40 commits intomasterfrom
v1.0.0

Conversation

@germanattanasio
Copy link
Copy Markdown
Contributor

Version 1.0 introduces automatically generated service classes for several services.

Services that are not affected in this release:

  • Speech to Text
  • Text to Speech
  • Retrieve and Rank
  • Document Conversion
  • Tradeoff Analytics

Services that are affected in this release:

  • Discovery
  • Conversation
  • Natural Language Understanding
  • Natural Language Classifier
  • Tone Analyzer
  • Language Translator
  • Personality Insights
  • Visual Recognition

Upon upgrading the SDK, follow the migration guide here.

Mike Kistler and others added 3 commits October 23, 2017 17:33
Rename core service file to watson_service.py
* Return None when status_code is 204 (No Content)

* Generated Python SDK

* Hand-coded changes to create/updateClassifier

* Updated tests for generated SDK

* Updated examples for generated SDK

* Updates from PR review

* Update discovery tests

* ignore vscode

* 💚 ignore personality insights v3 failing test

* 🔮 Add travis file to prevent encoding issues

* Hand-coded changes to PIV3 profile for text/csv accept type

* Fix and re-enable PIV3 test for csv return type

* 📝 Documentation changes for v1.0

* Add the Download Times link back

* Update examples and pylint errors

* Unicode to fix travis issue with python 2.7

* Revert unicode for v2.7
germanattanasio and others added 2 commits October 24, 2017 17:36
* Generated Python SDK for multiple services

* Hand-coded changes to create/updateClassifier

* Hand-coded changes to PIV3 profile for text/csv accept type
global_payload = {
'workspace_id': workspace_id,
'input': {
'text': "I am happy"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Line 47 under invokeToneConversation says:

invokeToneConversation calls the the Tone Analyzer service to get the

extra the is not intentional. right?

Comment thread README.md Outdated

## CHANGELOG
## Changes for v4.0
Version 4.0 focuses on the move to programmatically-generated code for many of the services. See the [changelog](https://github.com/watson-developer-cloud/python-sdk/wiki/Changelog) for the details.
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.

Should this be v1.0 instead of v4.0?

audio_file, content_type='audio/wav', timestamps=True,
word_confidence=True),
indent=2))
indent=2))
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.

It doesn't change the meaning, but I suspect this extra indentation wasn't intended.

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.

This was a pylint complain, so added those indentation

with open(join(dirname(__file__),
'../resources/tone-example.json')) as tone_json:
tone = tone_analyzer.tone(json.load(tone_json)['text'], 'emotion')
tone = tone_analyzer.tone(json.load(tone_json)['text'], "text/plain")
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.

Looks like the tones list was removed. Based on an email thread, though, I think there's a chance it will be reintroduced to maintain backwards compatibility. Not sure if y'all want to hold off on this release until there's a resolution to that discussion.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good eye @glennrfisher! The tones parameter was reintroduced but is now optional, so it moved after the content_type parameter. That wouldn't really be backward compatible, but fortunately we don't have to worry about that for a new major release. And happily, this example is still correct even with tones being reintroduced.

@ehdsouza The reordering of the parameters might be worth mentioning in the migration guide.

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.

Updated the migration guide.

print(json.dumps(url_result, indent=2))

faces_result = visual_recognition.detect_faces(images_url=test_url)
faces_result = visual_recognition.detect_faces(parameters=json.dumps({'url': test_url}))
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.

It's unfortunate that the API for Visual Recognition expects a JSON string. Not sure what can be done to improve that without changing the service interface.

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.

:(

#file_path = join(dirname(__file__), '../resources/text.png')
#with open(file_path, 'rb') as image_file:
# text_results = visual_recognition.recognize_text(images_file=image_file)
# print(json.dumps(text_results, indent=2))
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.

Was this intended to be commented?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes -- the text recognition methods of VR are deprecated.

count,
'return_fields':
",".join(return_fields)
if isinstance(return_fields, list) else return_fields,
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.

This looks awkward, although it seems to work okay. I suspect yapf formatted the code this way.

I made a quick test file, and it works as expected:

# test.py
x = ['1','2','3']
params = {
    'list':
    ','.join(x)
    if True else 'hello, world'
}
print(params['list'])
> python test.py
1,2,3

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I noticed the same thing. It turns out that if any line in the object constructor has to break at the :, then yapf breaks all the lines consistently at the :. It looks awful but it works.

Mike Kistler and others added 2 commits October 25, 2017 16:56
Comment thread examples/discovery_v1.py Outdated

discovery = watson_developer_cloud.DiscoveryV1(
discovery = DiscoveryV1(
'2016-11-07',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using a latest version here would make more sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use 2017-10-16

parallel_corpus_content_type=None,
parallel_corpus_filename=None,
monolingual_corpus_content_type=None,
monolingual_corpus_filename=None):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we probably do not want the content_types or filename parameters here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see why this happened -- a typo in the annotations in the LT swagger.

def create_classifier(self,
metadata,
training_data,
training_metadata_filename=None,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This parameter should be named metadata_filename.

Mike Kistler and others added 2 commits October 26, 2017 10:12

def __init__(self, version, url=default_url, **kwargs):
WatsonDeveloperCloudService.__init__(self, 'conversation', url,
**kwargs)
Copy link
Copy Markdown
Contributor

@jsstylos jsstylos Oct 26, 2017

Choose a reason for hiding this comment

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

Should continue to have the logic of **kwargs to support x_watson_learning_opt_out, etc.

    def __init__(self, version, url=default_url, **kwargs):
        WatsonDeveloperCloudService.__init__(self, 'conversation', url,
                                             **kwargs)

Copy link
Copy Markdown

@mkistler mkistler Oct 26, 2017

Choose a reason for hiding this comment

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

You can do x_watson_learning_opt_out with the default headers. This was recently added.

@ehdsouza We should make sure this is in the migration guide.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also, **kwargs is no longer needed for use_vcap_services, because it defaults to True -- there’s never a case where you want to specify False. Credentials passed in always override vcap services. If you want vcap services, don’t pass credentials.

Mike Kistler and others added 9 commits November 4, 2017 11:47
…-helper

Add convert_model helper function and use this in generated code
…les-tests

Fix unicode error in examples tests
…xceptions

Create custom exception for API errors containing the status code
🔥 enable all commands in tox to build pages

💚 Test documentation deployment

Install the library before calling docs/publish.sh
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 7, 2017

Codecov Report

Merging #286 into master will decrease coverage by 47.19%.
The diff coverage is 37.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
- Coverage   85.29%   38.09%   -47.2%     
==========================================
  Files          24       24              
  Lines        1231     8187    +6956     
==========================================
+ Hits         1050     3119    +2069     
- Misses        181     5068    +4887
Impacted Files Coverage Δ
watson_developer_cloud/discovery_v1.py 29.1% <ø> (-69.07%) ⬇️
watson_developer_cloud/conversation_v1.py 39.92% <ø> (-60.08%) ⬇️
...veloper_cloud/natural_language_understanding_v1.py 31.26% <ø> (-68.74%) ⬇️
watson_developer_cloud/tradeoff_analytics_v1.py 100% <100%> (ø) ⬆️
watson_developer_cloud/__init__.py 100% <100%> (ø) ⬆️
watson_developer_cloud/dialog_v1.py 44.44% <100%> (ø) ⬆️
watson_developer_cloud/alchemy_language_v1.py 42.66% <100%> (ø) ⬆️
watson_developer_cloud/personality_insights_v2.py 76.19% <100%> (ø) ⬆️
watson_developer_cloud/retrieve_and_rank_v1.py 98.14% <100%> (ø) ⬆️
watson_developer_cloud/document_conversion_v1.py 96.87% <100%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eec665f...37c30a9. Read the comment docs.

@ehdsouza ehdsouza merged commit 254430e into master Nov 13, 2017
@germanattanasio
Copy link
Copy Markdown
Contributor Author

selfie-0

@germanattanasio germanattanasio deleted the v1.0.0 branch November 14, 2017 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

7 participants