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

Add more local pipeline operations #24

Merged
merged 2 commits into from
May 22, 2021

Conversation

replomancer
Copy link
Member

Description

Adds every other remaining method to LocalPipelineOperations (we split work with Zachery).

Affected Dependencies

None.

How has this been tested?

Unit tests added 🧹

Checklist

@replomancer replomancer added the Type: New Feature ➕ Introduction of a completely new addition to the codebase label May 18, 2021
pipeline_dp/pipeline_operations.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvadym dvadym left a comment

Choose a reason for hiding this comment

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

Thanks, it look great. I left a question and a suggestion

def count_per_element(self, col, stage_name: str):
pass
def count_per_element(self, col, stage_name: Optional[str] = None):
yield from collections.Counter(col).items()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like idea with Counter!

One question, in this line, is 'col' accessed the first time only when iterations over count_per_element() started?

What do you think about adding the 'lazyness' test:
Let 'exception_generator' is a generator which raise an exception on the first access to it

and then
def test_lazyness():
self.ops.map(exception_generator) # should be no exception
self.ops.map_tuple(exception_generator)
....

on implementing a new method it would be easy to add to the test and to ensure that the method is lazy

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool idea for testing!
I added a test like this and yes, count_per_element works as expected.

@chinmayshah99
Copy link
Member

@replomancer there are some conflicts. Can you please resolve them?

try:
operator(exceptions_generator_function(), *args)
except ZeroDivisionError:
self.fail(f"local {operator.__name__} is not lazy")
Copy link
Member Author

Choose a reason for hiding this comment

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

This function exists for tests to fail properly with AssertionError (assertNotRaises does not exists in unittest).

e.g. when filter implementation is broken:

        return list(filter(fn, col))

the fail looks like this:

tests/pipeline_operations_test.py:122: in assert_laziness
    self.fail(f"local {operator.__name__} is not lazy")
E   AssertionError: local filter is not lazy

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for explanation. I like that approach!

try:
operator(exceptions_generator_function(), *args)
except ZeroDivisionError:
self.fail(f"local {operator.__name__} is not lazy")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for explanation. I like that approach!

@dvadym dvadym merged commit 6c5ad70 into OpenMined:main May 22, 2021
@replomancer replomancer deleted the local_operations_adapter branch May 22, 2021 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: New Feature ➕ Introduction of a completely new addition to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants