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

ENH: Enable the ability to compute multiple pipelines #1974

Merged
merged 4 commits into from Oct 13, 2017

Conversation

Projects
None yet
3 participants
@lianga888
Contributor

lianga888 commented Oct 4, 2017

No description provided.

@lianga888 lianga888 requested a review from ssanderson Oct 4, 2017

@coveralls

This comment has been minimized.

coveralls commented Oct 4, 2017

Coverage Status

Coverage increased (+0.003%) to 87.219% when pulling f69371f on enable_multi_pipeline into 31447d6 on master.

@ssanderson

@lianga888 the changes here look reasonable, but I think something fishy is happening in the test here; it looks like there are a couple typos, but the test seems to still be passing, which leads me to think that it's not being run or not doing what we expect it to do.

exists for another attached pipeline.
"""
msg = (
"Pipeline name '{name}' already exists for another pipeline."

This comment has been minimized.

@ssanderson

ssanderson Oct 4, 2017

Member

You can also use {name!r} here instead of having quotes around the format unit.

This comment has been minimized.

@ssanderson

ssanderson Oct 4, 2017

Member

Might be helpful to include a suggestion of how to fix the issue here, e.g. "Tried to attach a pipeline named {name}, but there was already an attached pipeline with that name. Please use a different name for this pipeline."

self.expected_close(date, asset)
)
self.assertEqual(
volumes.loc[asset, 'close'],

This comment has been minimized.

@ssanderson

ssanderson Oct 4, 2017

Member

I think this column name is a typo. I'm surprised that this test currently passes.

)
self.assertEqual(
volumes.loc[asset, 'close'],
self.expected_close(date, asset)

This comment has been minimized.

@ssanderson

ssanderson Oct 4, 2017

Member

Should this be expected_volume?

@@ -181,6 +183,9 @@ def expected_close(self, date, asset):
lookup = self.adj_closes
return lookup.loc[date, asset]
def expected_volume(self, date, asset):

This comment has been minimized.

@ssanderson

ssanderson Oct 4, 2017

Member

I would expect this function to have to account for the split in the test data used by this test.

@lianga888 lianga888 force-pushed the enable_multi_pipeline branch from f69371f to d6fd82f Oct 5, 2017

@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2017

Coverage Status

Coverage decreased (-0.4%) to 86.772% when pulling d6fd82f on enable_multi_pipeline into 7cd6f6a on master.

@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2017

Coverage Status

Coverage increased (+0.003%) to 87.205% when pulling d6fd82f on enable_multi_pipeline into 7cd6f6a on master.

@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2017

Coverage Status

Coverage increased (+0.003%) to 87.205% when pulling c02b9f7 on enable_multi_pipeline into 7cd6f6a on master.

@lianga888 lianga888 force-pushed the enable_multi_pipeline branch from c02b9f7 to 1567b8b Oct 5, 2017

@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2017

Coverage Status

Coverage decreased (-0.4%) to 86.772% when pulling 1567b8b on enable_multi_pipeline into 7cd6f6a on master.

@coveralls

This comment has been minimized.

coveralls commented Oct 5, 2017

Coverage Status

Coverage increased (+0.003%) to 87.205% when pulling 1567b8b on enable_multi_pipeline into 7cd6f6a on master.

@lianga888

This comment has been minimized.

Contributor

lianga888 commented Oct 6, 2017

@ssanderson this PR is ready for another pass, at your convenience

@@ -143,8 +174,14 @@ def get(self, key, dt):
try:
return self._cache[key].unwrap(dt)
except Expired:
del self._cache[key]
raise KeyError(key)
# We can't handle safely handle the exception in this block

This comment has been minimized.

@ssanderson

ssanderson Oct 12, 2017

Member

If we're going to re-raise an exception, then we don't need to explicitly clear the exception in the cleanup function. The important thing is just that by the time we exit this function, the exception referencing the cached object is no longer in sys.exc_info.

@@ -119,7 +148,7 @@ def __init__(self, cache=None):
else:
self._cache = {}
def get(self, key, dt):
def get(self, key, dt, cleanup=basic_cleanup):

This comment has been minimized.

@ssanderson

ssanderson Oct 12, 2017

Member

I would probably make this a parameter to ExpiringCache instead of to get.

# We remove the above sources of references in reverse order:
# 3. Clear the traceback. This is no-op in Python 3.
exc_clear()

This comment has been minimized.

@ssanderson

ssanderson Oct 12, 2017

Member

I don't think this is necessary anymore. See note below.

# removing any references to it. There are at least three sources
# of references:
# 1. self._pipeline_cache holds a reference.

This comment has been minimized.

@ssanderson

ssanderson Oct 12, 2017

Member

This comment is referencing variables from TradingAlgorithm, which doesn't really make sense if this is a standalone function.

clear_dataframe_indexer_caches(cache[key]._unsafe_get_value())
# 1. Clear the reference to the `CachedObject`
del cache[key]

This comment has been minimized.

@ssanderson

ssanderson Oct 12, 2017

Member

I don't think the cleanup function should have to be responsible for clearing the cache key. That should happen unconditionally no matter what.

I think the right signature for the cleanup function is to just take the value wrapped in the CachedObject.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Oct 13, 2017

LGTM

@lianga888 lianga888 force-pushed the enable_multi_pipeline branch from 94364d2 to e018275 Oct 13, 2017

@coveralls

This comment has been minimized.

coveralls commented Oct 13, 2017

Coverage Status

Changes Unknown when pulling e018275 on enable_multi_pipeline into ** on master**.

@lianga888 lianga888 merged commit 45257eb into master Oct 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lianga888 lianga888 deleted the enable_multi_pipeline branch Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment