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

Pre 1.0 maintenance PR #19

Merged
merged 13 commits into from
Jul 12, 2018
Merged

Pre 1.0 maintenance PR #19

merged 13 commits into from
Jul 12, 2018

Conversation

sgillies
Copy link
Contributor

@sgillies sgillies commented Jul 10, 2018

Getting riomucho to a stable 1.0.0!

Sean C. Gillies added 2 commits July 10, 2018 14:31
Remove unneeded/untested code, add change log, new requirements
enumeration, remove empty setup.cfg.
@coveralls
Copy link

coveralls commented Jul 10, 2018

Coverage Status

Coverage increased (+11.1%) to 100.0% when pulling 8a78b56 on rasterio-won-oh into 24036c8 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+11.1%) to 100.0% when pulling e11fe28 on rasterio-won-oh into 24036c8 on master.

setup.py Outdated
packages=find_packages(exclude=["ez_setup", "examples", "tests"]),
include_package_data=True,
zip_safe=False,
install_requires=["click", "numpy", "rasterio>=1.0rc4"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the requirements file and install_requires have different purposes. I'm pulling the library requirements back in here.

def __exit__(self, ext_t, ext_v, trace):
if ext_t:
traceback.print_exc()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was untested code and I don't see how it would be used in production.

try:
srcs = [rio.open(i) for i in inpaths]
except:
return
Copy link
Contributor Author

@sgillies sgillies Jul 10, 2018

Choose a reason for hiding this comment

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

This silently swallows all exceptions and I think it's better to delete it.

return work_func(srcs, window, ij, global_args), window
except Exception as e:
traceback.print_exc()
raise e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, now I get why this code was here. Looking for a better way...

@sgillies
Copy link
Contributor Author

I found the better way: a decorator that wraps a child process's exception and carries the traceback to the parent. The only problem with it is that there seems to be a Python 3.4 specific bug causing the processing pool to hang: https://travis-ci.org/mapbox/rio-mucho/jobs/402774581#L549.

@sgillies
Copy link
Contributor Author

sgillies commented Jul 11, 2018

@dnomadb @perrygeo @normanb I propose to drop support for Python 3.4 in version 1.0.0 of riomucho. I've noted in the change log that only Python 3.5 and 3.6 have a fix for a multiprocessing bug that didn't affect 2.7.

Python 3.4 is only getting security fixes now, as far as I can see.

@sgillies sgillies self-assigned this Jul 11, 2018
@sgillies sgillies added this to the 1.0 milestone Jul 11, 2018
rasterio
click==6.7
numpy==1.14.5
rasterio==1.0rc5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make our Travis build requirements stable and reproducible.


@job
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The job decorator wraps up tracebacks that happen in child processes and reraises them, which is better than printing them because they can be caught and inspected in Python.

@@ -1,95 +1,146 @@
"""TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docstrings are a todo.

@sgillies
Copy link
Contributor Author

Nice side effect: builds finish in <3 mins now.

@sgillies sgillies removed the request for review from perrygeo July 11, 2018 21:18
Copy link
Contributor

@dnomadb dnomadb left a comment

Choose a reason for hiding this comment

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

Looks good so far -- we should try this version out in a riomucho-using library (pxm alpha, rio color maybe)

import click
import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't even think we are using numpy in __init__.py, are we?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Just in scripts/riomucho_utils.py)

return wrapper


def main_worker(inpaths, g_work_func, g_args):
"""TODO"""
global work_func
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This global isn't needed anymore, we now wrap the user function up in another class that gets passed to imap_unordered.

@sgillies
Copy link
Contributor Author

sgillies commented Jul 12, 2018

@dnomadb @perrygeo the latest release of rio-color checks out with rio-mucho 1.0dev1. I will merge!

@sgillies sgillies merged commit 846c711 into master Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants