Skip to content
This repository has been archived by the owner on Nov 6, 2024. It is now read-only.

Added skill-unitconversion #1046

Closed
wants to merge 2 commits into from
Closed

Conversation

traverseda
Copy link

@traverseda traverseda commented Jul 12, 2019

Name of your skill: Unit Conversion

Description:

https://github.com/traverseda/mycroft-skill-unitconversion

Use python-pint to do unit conversions.

Very light testing so far and can't do complicated conversions.

Checklist:

+[submodule "NAME OF YOUR SKILL"]
+	path = name-of-your-skill
+	url = URL.FOR.YOUR.SKILL.git

@krisgesling krisgesling added needs validation Needs validatiion by another Skills Team Member or Community Member trying it new New Skill (not update to existing Skill) labels Jul 24, 2019
@krisgesling
Copy link
Contributor

Hi traverseda,

Thanks for submitting your first Skill! 🎉

To be included in the Marketplace the Skill goes through three reviews by our Skill Testing Team. You can read more about these reviews and the overall Skills Acceptance Process here: https://mycroft.ai/documentation/skills/skills-acceptance-process/

If you have push any updates in the meantime be sure to run mycroft-msk submit /opt/mycroft/skills/your-skill-dir to update this pull request. Also feel free to post here or in the Skills channel on Chat if you have any questions.

Looking forward to trying out your Skill 🙂

@traverseda
Copy link
Author

One issue seems to be that the intents are matching too many things, I'd like to tighten that up. I had discussed making this a fallback skill instead, but it would be a lot better if the skill simply didn't over-zealously match to a bunch of different questions.

Perhaps you can weigh in on MycroftAI/padatious#4 ?

@krisgesling
Copy link
Contributor

Meta

  • Platform: Debian Linux
  • Mycroft-core version: 19.8.2
  • Who: @krisgesling / @gez-mycroft
  • Datestamp: 2019-11-07_14:43_ACST
  • Language and dialect of tester: English, Australian accent

0. Automated tests

Are all automated tests passing?

  • Skill tester - Jenkins
  • Continuous Integration - Travis-CI

1. Code Review - secure and stable

  • Code Quality
    Can you understand what the code is doing? Is there inline documentation? Do you have any concerns about this code running on your machine? Are there any performance issues such as nested or infinite loops? Do you have significant concerns about the overall code quality?
    NOTE: We do not enforce PEP8 Checks on Skills

All great. A few notes regarding updates to Mycroft's API.
Any references to intent_file_handler can now be replaced with intent_handler.
The logger is now included in the MycroftSkill Class and can be accessed using self.log so no longer needs to be imported separately.

  • Error Handling
    Are there any specific checks we make for error handling or graceful degradation?

  • Libraries
    Does the Skill include the correct libraries? Does it use too many libraries or dependencies?

  • Required Dependencies
    Check requirements.txt and requirements.sh - are the required dependencies listed? If requirements.sh is used, is some form of conditional processing done to match against multiple distros? Often Skill Authors will add requirements.txt using only an “library=1.x.x” instead of “library >=1.x.x”. Check to make sure that there is an equal or greater than in the requirements to help future-proof the Skill, unless a specific version is needed.

Action required: there's a small error in your manifest.yml. It requires a top-level key of dependencies. The following should work:

dependencies:
  python:
    - pint
    - sympy
  • Settings
    Is the settingsmeta file well laid out? If settings are not used, has the default file been deleted? If it is the default file, the first setting section will be called "Options << Name of section". >

NA

  • Integration Tests
    Does the skill include sufficient integration tests, included in the test/intent folder?

No tests included.

Action required: add at least one integration test. PR in bound with some suggestions.

  • Other Files
    Are there any other files included that are unnecessary or you are unsure of their function?

2. Information Review - accurate and understandable

This review checks the README for completeness - does it follow the README template and include all the relevant sections such as Intents, known issues, dependencies and so on?

  • Icon
    Does the Skill have an appropriate icon that is either included in the repo or linked from an appropriate place (eg raw.githack.com not privateicons.com or rawgit.com)?

  • Description
    Are the title, short description, and long description (under 'About') clear and concise? Do they provide enough information to understand what the Skill does? Does the title have the word "skill" included? This is strongly discouraged.

  • Examples
    Do the examples give you a clear understanding of how you can use the Skill? Is there a single example per dot-point?

There are a separate section of examples down lower, these won't be displayed in the Marketplace unless they're added to the list above.

  • Supported Devices
    If relevant, are the supported devices listed? An example might be a Skill that requires the screen of the Mark II. If the section is not present, all devices are considered supported.

  • Categories
    Is there at least one category listed? At least one category must be selected for the Skill to be displayed correctly in the Mycroft Marketplace.
    Is the bolded category the most appropriate for this Skill? The bold category is considered the primary category and will be used for display in the Marketplace, all others are secondary and used for search results.

  • Tags
    Are listed tags appropriate for this Skill?

NA

  • License
    Is an appropriate LICENSE file used in the repo - such as Apache-2.0 or GPL-3.0?

3. Functional Review - intuitive and expected

  • Installation

Check that the Skill installs using voice commands. Mycroft will get the user to confirm which Skill should be installed if there is ambiguity in Skill names - such as duplicate names. If possible, name the Skill so that there is minimal duplication and/or conflict. You should also verify that the Skill name can be verbally pronounced by speaking the Skill name into the Mycroft command line several times, and reading the resulting transcriptions. Suggest alternative Skill names if it is difficult to verbally pronounce the Skill name. Please provide confirmation that the Skill was successfully installed and by what means (voice or mycroft-msm install), as well as what utterance was detected when invoking the install voice command.

Install method: mycroft-msm install https://github.com/traverseda/mycroft-skill-unitconversion

Checking that STT transcribes correctly
Utterance: ['install unit conversion']

I didn't find any Skill named unit conversion
STT transcribing name perfectly.

  • Settings
    If skill includes a settingsmeta file - are the settings well laid out? Does the placeholder text make sense? This can also be checked on home.mycroft.ai/#/skill

  • Dialog

Check the dialog directory to ensure that from a voice user interface perspective the dialogs read well. Alway play every dialog phrase on the command line by doing say so that you can check how the dialog is spoken. It is a good idea to run the dialog phrases through mimic.

Sometimes the dialog files will need a small tweak such as a space between words, or extra vowel sounds, to sound realistic. Sometimes words don't render as expected and alternative wording should be used. Some of the tricks you might need to use include separating words by a space - such as sub sonic instead of subsonic, or broad cast instead of broadcast.

Skill Functions

For each function of the Skill add a new checkbox with the utterance used to invoke the functionality. Confirm the output and behaviour of each. If any setup is required to perform these tests please indicate this directly before the test is described.

  • "How many feet are in a meter?"

1 foot is 0.3 metres

  • "What is 3 kilometers in miles?"

4 kilometer is 1.86 miles

  • "How many kilometers in a lightyear?"

1 kilometer is 0.0 light_years

  • "How many kilometers in a leap year?"

I don't know what a "leapyear" is

  • "How many kilometers make up a light year"
2019-11-07 15:17:07.200 | ERROR    |  9145 | mycroft.skills.mycroft_skill.mycroft_skill:on_error:785 | An error occurred while processing a request in Unit Conversion Skill
Traceback (most recent call last):
  File "/home/kris/mycroft-core/mycroft/skills/mycroft_skill/event_container.py", line 66, in wrapper
    handler(message)
  File "/opt/mycroft/skills/mycroft-skill-unitconversion.traverseda/__init__.py", line 16, in handle_unit_conversion
    target=(message.data.get('prefixto',"")+message.data['unitto']).lstrip("a ")
KeyError: 'unitto'

This shouldn't really be possible given that unitTo and unitFrom are both required entities in the intent file. We'll need to look into that. In the meantime, it's maybe worth using the get method to access these attributes in case they don't exist. Then it will at least report an unknown unit error rather than general exception.

  • "How many light years are in a kilometer?"

I don't know what a "lightkilometer" is.

  • "What is one light year in kilometers?"
2019-11-07 15:55:16.060 | ERROR    |  9145 | mycroft.skills.mycroft_skill.mycroft_skill:on_error:785 | An error occurred while processing a request in Unit Conversion Skill
Traceback (most recent call last):
  File "/home/kris/mycroft-core/mycroft/skills/mycroft_skill/event_container.py", line 66, in wrapper
    handler(message)
  File "/opt/mycroft/skills/mycroft-skill-unitconversion.traverseda/__init__.py", line 24, in handle_unit_conversion
    result = round((count*ureg(source)).to(target),2)
  File "/home/kris/mycroft-core/.venv/lib/python3.7/site-packages/pint/quantity.py", line 1123, in __round__
    return self.__class__(round(self._magnitude, ndigits=ndigits), self._units)
TypeError: type Mul doesn't define __round__ method

Summary

Sorry it's taken way too long to get this fully reviewed. We've been very focused on Mark II development so reviews are taking much longer than usual.

There was a small issue with your dependencies definition in manifest.yml - described above.
I've written a few quick tests that I'll submit as a PR back to your repo, feel free to use them or not as you wish.
I managed to trigger a few exceptions, though I know you flagged issues with entity extraction so will make some time to take a look at that too.

It's a great Skill though, thanks!

@krisgesling krisgesling changed the base branch from 19.02 to 19.08 November 7, 2019 07:27
@traverseda
Copy link
Author

Yeah, I considered using a get method to access those attributes. I'd much rather fix the underlying issue though.

It also causes another issue, I haven't touched this in a few months, but if I recall correctly it was also matching inputs like "what is 1 plus 1", and that's a much bigger problem then it just not being able to recognize some units. I did have a solution that silenced those errors (using try-except) but ultimately scrapped it, as I wanted the problems to be clear during testing.

It might be worth including some null results in your testing, sentences that are close to something it should match but which it actually shouldn't match. Probably not necessary if podatious is working properly though.

@krisgesling krisgesling added waiting Waiting on the Skill Author to respond and removed needs validation Needs validatiion by another Skills Team Member or Community Member trying it labels Jan 8, 2020
@forslund
Copy link
Collaborator

forslund commented Nov 5, 2024

Project is no longer maintained, closing.

@forslund forslund closed this Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new New Skill (not update to existing Skill) waiting Waiting on the Skill Author to respond
Projects
No open projects
Status: Skill submission
Development

Successfully merging this pull request may close these issues.

3 participants