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

added extra info in rasa --version command #5726

Merged
merged 26 commits into from
May 7, 2020
Merged

Conversation

koaning
Copy link
Contributor

@koaning koaning commented Apr 27, 2020

Proposed changes:

This PR implements the changes proposed here. The idea is to add extra information to the rasa --version command so that it is less effort to file a bug report.

This is the new behavior.

> python -m rasa --version
Rasa Version     : 1.9.7
Python Version   : 3.6.8 (v3.6.8:3c6b436a57, Dec 24 2018, 02:04:31) 
Operating System : [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)]
Virtualen Env    : /Users/vincent/Development/rasa/venv
Rasa SDK Version : 1.9.0

If you don't have rasa-x locally it won't print it.

> python -m pip uninstall rasa-sdk
> python -m rasa --version      
Rasa Version     : 1.9.7
Python Version   : 3.6.8 (v3.6.8:3c6b436a57, Dec 24 2018, 02:04:31) 
Operating System : [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)]
Virtualen Env    : /Users/vincent/Development/rasa/venv

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@koaning koaning added the area:rasa-oss 🎡 Anything related to the open source Rasa framework label Apr 27, 2020
@koaning
Copy link
Contributor Author

koaning commented Apr 27, 2020

CI seems to complain about lines that are in master, found here. I could remove them, but I left them because I assumed they were there for a reason.

@koaning
Copy link
Contributor Author

koaning commented Apr 27, 2020

Fixed a bug such that this will work outside of a virtualenv. The previous implementation looked at the environment variable for python path. It is now retreived from sys.path.

>  rasa --version
Rasa Version     : 1.10.0a3
Python Version   : 3.6.8 (v3.6.8:3c6b436a57, Dec 24 2018, 02:04:31) 
Operating System : [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)]
Python Path      : /Users/vincent/Development/rasa/venv/bin
Rasa SDK Version : 1.9.0

The CI was complaining that coverage went down because of the few lines that I added so I added a unit test that checks if certain properties are reported.

@koaning koaning changed the title added extra info in version added extra info in rasa --version command Apr 27, 2020
@koaning
Copy link
Contributor Author

koaning commented Apr 28, 2020

Mhmmm....

 FAILED: /home/runner/work/rasa/rasa/.pytype/pyi/rasa/__main__.pyi 
/home/runner/.cache/pypoetry/virtualenvs/rasa-wb3tDUZd-py3.7/bin/python -m pytype.single --imports_info /home/runner/work/rasa/rasa/.pytype/imports/rasa.__main__.imports --module-name rasa.__main__ -V 3.7 -o /home/runner/work/rasa/rasa/.pytype/pyi/rasa/__main__.pyi --analyze-annotated --nofail --quick /home/runner/work/rasa/rasa/rasa/__main__.py
File "/home/runner/work/rasa/rasa/rasa/__main__.py", line 81, in print_version: Can't find module 'rasax.community.version'. [import-error]

I get why the Code Quality tool is complaining here, but then again, I don't see a way around this. We need to try to import something and we don't know if it is installed. I could remove rasa-x here since some of the time this information is not fetched from the command line but it feels like this command show list all relevant items. Would love to get advice on this. @erohmensing?

@koaning
Copy link
Contributor Author

koaning commented Apr 30, 2020

@erohmensing I have now removed the check for 'Rasa X'. It was giving build errors, but since typically people do not have rasa-x locally installed anyway it felt like something best removed.

Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Let me take a look at rasa x -- probably just an issue of where the version is found?

Can we put all of the info-gathering code before the prints please?

rasa/__main__.py Outdated Show resolved Hide resolved
@erohmensing
Copy link
Contributor

@koaning maybe we can make pytype ignore the import error then? google/pytype#163

@koaning
Copy link
Contributor Author

koaning commented May 4, 2020

The DeepSource: Python is currently complaining about these lines of code.

It is importing os and sys inside of a function that does not have a docstring is the complaint. I could fix it, but the comment # Running as a standalone python application makes me think twice about it.

In the meantime I have moved back Rasa X and I seem to have fixed the checking issue by adding # pytype: disable=import-error inline.

This is what the command currently does on my machine;

> python -m rasa --version         
Rasa Version     : 1.10.0
Rasa SDK Version : 1.9.0
Rasa X Version   : None
Python Version   : 3.6.8 (v3.6.8:3c6b436a57, Dec 24 2018, 02:04:31) 
Operating System : [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)]
Python Path      : /Users/vincent/Development/rasa/venv/bin/python

@erohmensing
Copy link
Contributor

It's fine to not always do exactly what deepsource says, though i was going to recommend a docstring because it's not just putting out the rasa version as one might expect

@erohmensing
Copy link
Contributor

Sorry to clarify -- if deepsource is yelling at code that's not yours, don't worry about it. But a docstring for version() might be nice.

@koaning
Copy link
Contributor Author

koaning commented May 7, 2020

@erohmensing I think everything is now covered. The deepsource still complains about lines of code outside of the ones I made. I added code changes based on your comments (thanks for those by the way!).

Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Nice! Just some style stuff and the OS thing we talked about

rasa/__main__.py Outdated Show resolved Hide resolved
rasa/__main__.py Outdated Show resolved Hide resolved
rasa/__main__.py Outdated Show resolved Hide resolved
changelog/5637.enhancement.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Awesome, I think this is going to be super helpful

rasa/__main__.py Outdated Show resolved Hide resolved
rasa/__main__.py Outdated Show resolved Hide resolved
print(f"Rasa SDK Version : {rasa_sdk_version}")
print(f"Rasa X Version : {rasa_x_info}")
print(f"Python Version : {python_version}")
print(f"Operating System : {platform.system()}")
Copy link
Contributor

Choose a reason for hiding this comment

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

why not platform.platform()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gives slightly more information that could potentially be relevant, like that the system is 64bit. It's not super needed but I figured it was more complete.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, platform.system() does? Because for me:

In [1]: import platform

In [2]: platform.platform()
Out[2]: 'Darwin-19.3.0-x86_64-i386-64bit'

In [3]: platform.system()
Out[3]: 'Darwin'

I would also like the slightly more information :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crud, mybad! Somehow I merged the whole thing with system instead of platform. I've added a quick fix PR here: #5793

koaning and others added 2 commits May 7, 2020 17:01
Co-authored-by: Ella Rohm-Ensing <[email protected]>
Co-authored-by: Ella Rohm-Ensing <[email protected]>
@koaning koaning merged commit 88aa903 into master May 7, 2020
@koaning koaning deleted the rasa-info-command branch May 7, 2020 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants