python 3 and siggen 1.0.0+ support #5

willkg · 2019-05-23T21:14:20Z

Fixes #3, #4.

* update to support Python 3 and siggen 1.0.0+ * fixes example so it works * adds test and example to README

willkg · 2019-05-23T21:15:20Z

.gitignore

+*~
+*.swp
+__pycache__
+fx_crash_sig.egg-info/


.gitignore helps deal with random files that shouldn't get checked in. I threw things that affected me in here.

willkg · 2019-05-23T21:15:40Z

README.md

+
+```sh
+python setup.py test
+```


I think this is how to run the tests. It ran the tests for me. If this isn't, then let me know and I can adjust this.

willkg · 2019-05-23T21:16:34Z

README.md

+
+```sh
+python example.py
+```


Given that there's an example.py module that shows how to do some things, I thought I'd surface that.

To run it, you either have to have . on your PYTHONPATH or we needed to bump it up to the parent directory. I did the latter figuring it's easier to document.

Let me know if you'd rather we did something different.

That's good with me. The Example script link above was broken though.

willkg · 2019-05-23T21:16:58Z

example.py

    print(symbolicated)

    if symbolicated is not None:
-        signature = crash_processor.get_signature_from_symbolicated(symbolicated)
-        print(signature)
+        result = crash_processor.get_signature_from_symbolicated(symbolicated)


siggen 0.2.0 changed the API so it returns a Result now. I adjusted the code accordingly.

willkg · 2019-05-23T21:17:22Z

fx_crash_sig/__init__.py

+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+SYMBOLS_API = 'https://symbols.mozilla.org/symbolicate/v5'


SYMBOLS_API was defined in a couple of places. I lifted it to this module.

willkg · 2019-05-23T21:17:53Z

fx_crash_sig/tests.py

@@ -24,7 +28,7 @@ def test_symbolicate_single2(self):

    def test_symbolicate_none(self):
        symbolicated = self.symbolicator.symbolicate(None)
-        self.assertEquals(symbolicated, {})
+        self.assertEqual(symbolicated, {})


assertEquals is deprecated in favor of assertEqual (with no s) so I fixed that.

willkg · 2019-05-23T21:18:25Z

requirements.txt

@@ -1,3 +1,3 @@
 requests==2.19.1
-siggen==0.1.2
+siggen<2


If we restrict it to <2, then it'll pick up the latest version that's below major version 2. I'll keep to semantic versioning, so that should work and won't break the API.

Wouldn't it be safer and more predictable to specify a single version? Is the reasoning for this so that signature lists can be kept up to date?

I just read #4 so this makes sense to me.

This is one of those tough decisions: what to pin on. I was thinking that we control siggen, so we can make sure that anything under 2.0.0 guarantees API backwards compatibility. I think with that guarantee, it's safe to do this here and doing this here means we don't have to do an fx-crash-sig update for every siggen update. I think we want to have to do fewer fx-crash-sig updates.

Having said that, I don't really understand the Telemetry code or databricks or how this would get used. Is it likely that one of those systems will cache fx-crash-sig? In which case if we don't increment the fx-crash-sig version, they won't pick up newer versions of siggen, either?

If I remember correctly databricks just does a pip install for each new cluster without caching so I think in this case it would install the latest siggen even if fx-crash-sig isn't updated. You may want to double check that with someone else.

willkg · 2019-05-23T21:20:28Z

setup.py

@@ -36,7 +36,7 @@ def read_file(name):
    """,
    classifiers=[
        'License :: OSI Approved :: Mozilla Public License 2.0 (MPL 2.0)',
-        'Programming Language :: Python :: 2 :: Only',
+        'Programming Language :: Python :: 3 :: Only',


siggen 1.0.0 is Python 3 only. Because of that, this effectively becomes Python 3 only. Python 2 is unsupported as of the end of this year and supporting Python 2 and 3 is a maintenance burden, so I'd rather we just stuck with Python 3.

If that turns out to be an unfeasible, we can figure something out.

I don't expect any issues with python 3 only. This library is fairly simple and machines using this should have 3.

willkg · 2019-05-23T21:21:43Z

This doesn't surface debugging information. I was thinking of adding a get_debug_info() function that returns the version of Python, fx_crash_sig, and siggen as a dict. Seems like it'd reduce the work it takes to support this, but maybe it won't actually do that.

BenWu · 2019-05-29T22:38:20Z

For debug info the command line util could just take a -V argument that prints whatever you need.

The changes here seem fine to me, just a couple of comments. I can give you access to the repo so you can merge it yourself if you'd like.

Does this also fix #4?

willkg · 2019-05-30T01:37:11Z

Oh, I like the idea of a -V. I'll add that.

This adds get_version_info() which returns the versions for siggen, fix-crash-sig and Python. This will greatly ease debugging issues. This also adds a --version flag to fx-crash-sig command which spits those things out to stdout.

willkg · 2019-05-30T01:56:06Z

I added --version instead of -V. I decided -v for verbose was too close and it's probably less likely someone will want the version.

Does that look ok?

BenWu · 2019-05-30T02:18:14Z

fx_crash_sig/crash_processor.py

 from siggen.generator import SignatureGenerator

 from fx_crash_sig import SYMBOLS_API
 from fx_crash_sig.symbolicate import Symbolicator


+def get_version_info():


Should get_version_info be in crash_processor.py? I think it would make more sense to just be in cmd_get_crash_sig.py or __init__.py. If you don't agree you can just leave it.

It's better if it's in the library rather than just in the command. Then if someone uses fx-crash-sig like a library and not like a command, it's still available. So I think it shouldn't be in cmd_get_crash_sig.py.

I'll move it to __init__.py.

BenWu · 2019-05-30T02:20:00Z

fx_crash_sig/cmd_get_crash_sig.py

+    if args.version:
+        version_info = get_version_info()
+        for key in sorted(version_info):
+            print('%s: %s' % (key, version_info[key]))


I would prefer .format() over %.

willkg · 2019-05-31T00:08:22Z

@ben-wu ^^^ How's that look?

BenWu

Just two small things but otherwise everything looks good to me.

BenWu · 2019-05-31T02:31:14Z

fx_crash_sig/__init__.py

+        fx_crash_sig_version = 'unknown'
+
+    return {
+        'siggen': '%s (%s)' % (siggen_version, siggen_releasedate),


.format() here too. I forgot to mention it last time.

Whoops--I missed this. Sorry about that!

BenWu · 2019-05-31T02:35:00Z

README.md

@@ -34,3 +34,14 @@ Command line (using [sample.json](/sample.json)):
 ```sh


On line 20 above can you change the link to /example.py? I can't comment on the actual line.

willkg · 2019-05-31T17:17:46Z

^^^ That should cover the request changes. Sorry about that!

BenWu

Looks good to me. Do you want to merge this now?

willkg · 2019-05-31T20:59:41Z

Done! Thank you!

willkg added 3 commits May 23, 2019 16:45

Add setup.cfg with flake8 linting configuration

f851124

Add .gitignore

6c6d8c5

Update to Python 3, fix example

a06f5f3

* update to support Python 3 and siggen 1.0.0+ * fixes example so it works * adds test and example to README

willkg commented May 23, 2019

View reviewed changes

Remove commented out bits in setup.cfg

1e951c8

willkg commented May 23, 2019

View reviewed changes

Add get_version_info and --version flag

34835a7

This adds get_version_info() which returns the versions for siggen, fix-crash-sig and Python. This will greatly ease debugging issues. This also adds a --version flag to fx-crash-sig command which spits those things out to stdout.

BenWu reviewed May 30, 2019

View reviewed changes

willkg added 2 commits May 30, 2019 20:06

Move get_version_info() to __init__.py

06a6968

Switch from % to .format()

0377494

BenWu suggested changes May 31, 2019

View reviewed changes

Switch % for .format() and fix link in README

b78dbff

BenWu approved these changes May 31, 2019

View reviewed changes

willkg merged commit 33e0f3e into mozilla-services:master May 31, 2019

BenWu mentioned this pull request Jun 1, 2019

pin siggen version and surface it somehow #4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python 3 and siggen 1.0.0+ support #5

python 3 and siggen 1.0.0+ support #5

willkg commented May 23, 2019 •

edited

Loading

willkg May 23, 2019

willkg May 23, 2019

willkg May 23, 2019

BenWu May 29, 2019

willkg May 23, 2019

willkg May 23, 2019

willkg May 23, 2019

willkg May 23, 2019

BenWu May 29, 2019

BenWu May 29, 2019

willkg May 30, 2019

BenWu May 30, 2019

willkg May 23, 2019

BenWu May 29, 2019 •

edited

Loading

willkg commented May 23, 2019

BenWu commented May 29, 2019

willkg commented May 30, 2019

willkg commented May 30, 2019

BenWu May 30, 2019

willkg May 31, 2019

BenWu May 30, 2019

willkg May 31, 2019

willkg commented May 31, 2019

BenWu left a comment

BenWu May 31, 2019

willkg May 31, 2019

BenWu May 31, 2019

willkg commented May 31, 2019

BenWu left a comment

willkg commented May 31, 2019

		@@ -34,3 +34,14 @@ Command line (using [sample.json](/sample.json)):
		```sh

python 3 and siggen 1.0.0+ support #5

python 3 and siggen 1.0.0+ support #5

Conversation

willkg commented May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWu May 29, 2019 • edited Loading

Choose a reason for hiding this comment

willkg commented May 23, 2019

BenWu commented May 29, 2019

willkg commented May 30, 2019

willkg commented May 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willkg commented May 31, 2019

BenWu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willkg commented May 31, 2019

BenWu left a comment

Choose a reason for hiding this comment

willkg commented May 31, 2019

willkg commented May 23, 2019 •

edited

Loading

BenWu May 29, 2019 •

edited

Loading