From 2dd8089409a9d9f7aab17faa5b68ff6895e6990a Mon Sep 17 00:00:00 2001 From: Ajit Singh <99013051+ajit97singh@users.noreply.github.com> Date: Mon, 30 Sep 2024 19:28:05 +0530 Subject: [PATCH] chore: Removed deprecated max_loop_allowed argument from Pipeline init (#8409) * Added equality check for sender and receiver in connection function of pipeline * Update base.py irrelevant changes reverted * added release note * removed deprecated param max_loops_allowed from pipeline init * added release note * revert non relevant test * Delete releasenotes/notes/remove-support-to-connect-component-to-self-6eedfb287f2a2a02.yaml * revery non relevant change * Remove unused test_pipeline_deprecated.yaml * Remove PipelineMaxLoops error * Update release notes --------- Co-authored-by: Silvano Cerza --- haystack/core/errors.py | 16 +- haystack/core/pipeline/base.py | 47 +----- haystack/core/pipeline/pipeline.py | 1 - ...-param-from-pipeline-6b05ce1ff1f7fdec.yaml | 6 + test/core/pipeline/features/README.md | 1 - test/core/pipeline/test_pipeline.py | 143 +----------------- test/core/pipeline/test_tracing.py | 2 - test/core/test_serialization.py | 1 - .../yaml/test_pipeline_deprecated.yaml | 14 -- 9 files changed, 13 insertions(+), 218 deletions(-) create mode 100644 releasenotes/notes/remove-deprecated-param-from-pipeline-6b05ce1ff1f7fdec.yaml delete mode 100644 test/test_files/yaml/test_pipeline_deprecated.yaml diff --git a/haystack/core/errors.py b/haystack/core/errors.py index b312f6723..bd76fbcce 100644 --- a/haystack/core/errors.py +++ b/haystack/core/errors.py @@ -2,11 +2,6 @@ # # SPDX-License-Identifier: Apache-2.0 -import warnings - -# TODO: Remove this when PipelineMaxLoops is removed -warnings.filterwarnings("default", category=DeprecationWarning, module=__name__) - class PipelineError(Exception): pass @@ -28,16 +23,7 @@ class PipelineDrawingError(PipelineError): pass -class PipelineMaxLoops(PipelineError): - # NOTE: This is shown also when importing PipelineMaxComponentRuns, I can't find an easy - # way to fix this, so I will ignore that case. - warnings.warn( - "PipelineMaxLoops is deprecated and will be remove in version '2.7.0'; use PipelineMaxComponentRuns instead.", - DeprecationWarning, - ) - - -class PipelineMaxComponentRuns(PipelineMaxLoops): +class PipelineMaxComponentRuns(PipelineError): pass diff --git a/haystack/core/pipeline/base.py b/haystack/core/pipeline/base.py index 7fa8bc1eb..a2c0306de 100644 --- a/haystack/core/pipeline/base.py +++ b/haystack/core/pipeline/base.py @@ -42,11 +42,6 @@ T = TypeVar("T", bound="PipelineBase") logger = logging.getLogger(__name__) -_MAX_LOOPS_ALLOWED_DEPRECATION_MESSAGE = ( - "'max_loops_allowed' argument is deprecated and will be removed in version '2.7.0'. " - "Use 'max_runs_per_component' instead." -) - class PipelineBase: """ @@ -58,7 +53,6 @@ class PipelineBase: def __init__( self, metadata: Optional[Dict[str, Any]] = None, - max_loops_allowed: Optional[int] = None, debug_path: Optional[Union[Path, str]] = None, max_runs_per_component: int = 100, ): @@ -68,9 +62,6 @@ class PipelineBase: :param metadata: Arbitrary dictionary to store metadata about this `Pipeline`. Make sure all the values contained in this dictionary can be serialized and deserialized if you wish to save this `Pipeline` to file. - :param max_loops_allowed: - How many times the `Pipeline` can run the same node before throwing an exception. - This is deprecated and will be removed in version 2.7.0, use `max_runs_per_component` instead. :param debug_path: When debug is enabled in `run()`, where to save the debug data. :param max_runs_per_component: @@ -91,35 +82,7 @@ class PipelineBase: ) self._debug_path = Path(debug_path) - if max_loops_allowed is not None: - warnings.warn(_MAX_LOOPS_ALLOWED_DEPRECATION_MESSAGE, DeprecationWarning) - self._max_runs_per_component = max_loops_allowed - else: - self._max_runs_per_component = max_runs_per_component - - @property - def max_loops_allowed(self) -> int: - """ - Returns the maximum number of runs per Component allowed in this Pipeline. - - This is a deprecated field, use `max_runs_per_component` instead. - - :return: Maximum number of runs per Component - """ - warnings.warn(_MAX_LOOPS_ALLOWED_DEPRECATION_MESSAGE, DeprecationWarning) - return self._max_runs_per_component - - @max_loops_allowed.setter - def max_loops_allowed(self, value: int): - """ - Sets the maximum number of runs per Component allowed in this Pipeline. - - This is a deprecated property, use `max_runs_per_component` instead. - - :param value: Maximum number of runs per Component - """ - warnings.warn(_MAX_LOOPS_ALLOWED_DEPRECATION_MESSAGE, DeprecationWarning) - self._max_runs_per_component = value + self._max_runs_per_component = max_runs_per_component def __eq__(self, other) -> bool: """ @@ -199,14 +162,8 @@ class PipelineBase: data_copy = deepcopy(data) # to prevent modification of original data metadata = data_copy.get("metadata", {}) max_runs_per_component = data_copy.get("max_runs_per_component", 100) - max_loops_allowed = data_copy.get("max_loops_allowed", None) debug_path = Path(data_copy.get("debug_path", ".haystack_debug/")) - pipe = cls( - metadata=metadata, - max_loops_allowed=max_loops_allowed, - max_runs_per_component=max_runs_per_component, - debug_path=debug_path, - ) + pipe = cls(metadata=metadata, max_runs_per_component=max_runs_per_component, debug_path=debug_path) components_to_reuse = kwargs.get("components", {}) for name, component_data in data_copy.get("components", {}).items(): if name in components_to_reuse: diff --git a/haystack/core/pipeline/pipeline.py b/haystack/core/pipeline/pipeline.py index 62e7c8f3d..ce3b1c1f6 100644 --- a/haystack/core/pipeline/pipeline.py +++ b/haystack/core/pipeline/pipeline.py @@ -205,7 +205,6 @@ class Pipeline(PipelineBase): "haystack.pipeline.input_data": data, "haystack.pipeline.output_data": final_outputs, "haystack.pipeline.metadata": self.metadata, - "haystack.pipeline.max_loops_allowed": self._max_runs_per_component, "haystack.pipeline.max_runs_per_component": self._max_runs_per_component, }, ): diff --git a/releasenotes/notes/remove-deprecated-param-from-pipeline-6b05ce1ff1f7fdec.yaml b/releasenotes/notes/remove-deprecated-param-from-pipeline-6b05ce1ff1f7fdec.yaml new file mode 100644 index 000000000..783382cb8 --- /dev/null +++ b/releasenotes/notes/remove-deprecated-param-from-pipeline-6b05ce1ff1f7fdec.yaml @@ -0,0 +1,6 @@ +--- +upgrade: + - | + Removed `Pipeline` init argument `max_loops_allowed`. Use `max_runs_per_component` instead. + - | + Removed `PipelineMaxLoops` exception. Use `PipelineMaxComponentRuns` instead. diff --git a/test/core/pipeline/features/README.md b/test/core/pipeline/features/README.md index 7a5abc4df..4d3c362cc 100644 --- a/test/core/pipeline/features/README.md +++ b/test/core/pipeline/features/README.md @@ -115,7 +115,6 @@ def pipeline_that_has_an_infinite_loop(): component.set_output_types(self, a=int, b=int) FakeComponent = component_class("FakeComponent", output={"a": 1, "b": 1}, extra_fields={"__init__": custom_init}) - pipe = Pipeline(max_loops_allowed=1) pipe.add_component("first", FakeComponent()) pipe.add_component("second", FakeComponent()) pipe.connect("first.a", "second.x") diff --git a/test/core/pipeline/test_pipeline.py b/test/core/pipeline/test_pipeline.py index 95d90e51a..3a07c049f 100644 --- a/test/core/pipeline/test_pipeline.py +++ b/test/core/pipeline/test_pipeline.py @@ -56,15 +56,6 @@ class TestPipeline: It doesn't test Pipeline.run(), that is done separately in a different way. """ - def test_pipeline_dumps_with_deprecated_max_loops_allowed(self, test_files_path): - pipeline = Pipeline(max_loops_allowed=99) - pipeline.add_component("Comp1", FakeComponent("Foo")) - pipeline.add_component("Comp2", FakeComponent()) - pipeline.connect("Comp1.value", "Comp2.input_") - result = pipeline.dumps() - with open(f"{test_files_path}/yaml/test_pipeline.yaml", "r") as f: - assert f.read() == result - def test_pipeline_dumps(self, test_files_path): pipeline = Pipeline(max_runs_per_component=99) pipeline.add_component("Comp1", FakeComponent("Foo")) @@ -74,14 +65,6 @@ class TestPipeline: with open(f"{test_files_path}/yaml/test_pipeline.yaml", "r") as f: assert f.read() == result - def test_pipeline_loads_with_deprecated_max_loops_allowed(self, test_files_path): - with open(f"{test_files_path}/yaml/test_pipeline_deprecated.yaml", "r") as f: - pipeline = Pipeline.loads(f.read()) - assert pipeline.max_loops_allowed == 99 - assert pipeline._max_runs_per_component == 99 - assert isinstance(pipeline.get_component("Comp1"), FakeComponent) - assert isinstance(pipeline.get_component("Comp2"), FakeComponent) - def test_pipeline_loads_invalid_data(self): invalid_yaml = """components: Comp1: @@ -95,7 +78,6 @@ class TestPipeline: connections: * receiver: Comp2.input_ sender: Comp1.value - max_loops_allowed: 99 metadata: """ @@ -114,24 +96,12 @@ class TestPipeline: connections: - receiver: Comp2.input_ sender: Comp1.value - max_loops_allowed: 99 metadata: {} """ with pytest.raises(DeserializationError, match=".*Comp1.*unknown.*"): pipeline = Pipeline.loads(invalid_init_parameter_yaml) - def test_pipeline_dump_with_deprecated_max_loops_allowed(self, test_files_path, tmp_path): - pipeline = Pipeline(max_loops_allowed=99) - pipeline.add_component("Comp1", FakeComponent("Foo")) - pipeline.add_component("Comp2", FakeComponent()) - pipeline.connect("Comp1.value", "Comp2.input_") - with open(tmp_path / "out.yaml", "w") as f: - pipeline.dump(f) - # re-open and ensure it's the same data as the test file - with open(f"{test_files_path}/yaml/test_pipeline.yaml", "r") as test_f, open(tmp_path / "out.yaml", "r") as f: - assert f.read() == test_f.read() - def test_pipeline_dump(self, test_files_path, tmp_path): pipeline = Pipeline(max_runs_per_component=99) pipeline.add_component("Comp1", FakeComponent("Foo")) @@ -143,18 +113,9 @@ class TestPipeline: with open(f"{test_files_path}/yaml/test_pipeline.yaml", "r") as test_f, open(tmp_path / "out.yaml", "r") as f: assert f.read() == test_f.read() - def test_pipeline_load_with_deprecated_max_loops_allowed(self, test_files_path): - with open(f"{test_files_path}/yaml/test_pipeline_deprecated.yaml", "r") as f: - pipeline = Pipeline.load(f) - assert pipeline.max_loops_allowed == 99 - assert pipeline._max_runs_per_component == 99 - assert isinstance(pipeline.get_component("Comp1"), FakeComponent) - assert isinstance(pipeline.get_component("Comp2"), FakeComponent) - def test_pipeline_load(self, test_files_path): with open(f"{test_files_path}/yaml/test_pipeline.yaml", "r") as f: pipeline = Pipeline.load(f) - assert pipeline.max_loops_allowed == 99 assert pipeline._max_runs_per_component == 99 assert isinstance(pipeline.get_component("Comp1"), FakeComponent) assert isinstance(pipeline.get_component("Comp2"), FakeComponent) @@ -288,7 +249,7 @@ class TestPipeline: # UNIT def test_repr(self): - pipe = Pipeline(metadata={"test": "test"}, max_loops_allowed=42) + pipe = Pipeline(metadata={"test": "test"}) pipe.add_component("add_two", AddFixedValue(add=2)) pipe.add_component("add_default", AddFixedValue()) pipe.add_component("double", Double()) @@ -315,7 +276,7 @@ class TestPipeline: add_two = AddFixedValue(add=2) add_default = AddFixedValue() double = Double() - pipe = Pipeline(metadata={"test": "test"}, max_loops_allowed=42) + pipe = Pipeline(metadata={"test": "test"}, max_runs_per_component=42) pipe.add_component("add_two", add_two) pipe.add_component("add_default", add_default) pipe.add_component("double", double) @@ -344,86 +305,6 @@ class TestPipeline: } assert res == expected - # UNIT - def test_from_dict_with_deprecated_max_loops_allowed(self): - data = { - "metadata": {"test": "test"}, - "max_loops_allowed": 101, - "components": { - "add_two": { - "type": "haystack.testing.sample_components.add_value.AddFixedValue", - "init_parameters": {"add": 2}, - }, - "add_default": { - "type": "haystack.testing.sample_components.add_value.AddFixedValue", - "init_parameters": {"add": 1}, - }, - "double": {"type": "haystack.testing.sample_components.double.Double", "init_parameters": {}}, - }, - "connections": [ - {"sender": "add_two.result", "receiver": "double.value"}, - {"sender": "double.value", "receiver": "add_default.value"}, - ], - } - pipe = Pipeline.from_dict(data) - - assert pipe.metadata == {"test": "test"} - assert pipe.max_loops_allowed == 101 - assert pipe._max_runs_per_component == 101 - - # Components - assert len(pipe.graph.nodes) == 3 - ## add_two - add_two = pipe.graph.nodes["add_two"] - assert add_two["instance"].add == 2 - assert add_two["input_sockets"] == { - "value": InputSocket(name="value", type=int), - "add": InputSocket(name="add", type=Optional[int], default_value=None), - } - assert add_two["output_sockets"] == {"result": OutputSocket(name="result", type=int, receivers=["double"])} - assert add_two["visits"] == 0 - - ## add_default - add_default = pipe.graph.nodes["add_default"] - assert add_default["instance"].add == 1 - assert add_default["input_sockets"] == { - "value": InputSocket(name="value", type=int, senders=["double"]), - "add": InputSocket(name="add", type=Optional[int], default_value=None), - } - assert add_default["output_sockets"] == {"result": OutputSocket(name="result", type=int)} - assert add_default["visits"] == 0 - - ## double - double = pipe.graph.nodes["double"] - assert double["instance"] - assert double["input_sockets"] == {"value": InputSocket(name="value", type=int, senders=["add_two"])} - assert double["output_sockets"] == {"value": OutputSocket(name="value", type=int, receivers=["add_default"])} - assert double["visits"] == 0 - - # Connections - connections = list(pipe.graph.edges(data=True)) - assert len(connections) == 2 - assert connections[0] == ( - "add_two", - "double", - { - "conn_type": "int", - "from_socket": OutputSocket(name="result", type=int, receivers=["double"]), - "to_socket": InputSocket(name="value", type=int, senders=["add_two"]), - "mandatory": True, - }, - ) - assert connections[1] == ( - "double", - "add_default", - { - "conn_type": "int", - "from_socket": OutputSocket(name="value", type=int, receivers=["add_default"]), - "to_socket": InputSocket(name="value", type=int, senders=["double"]), - "mandatory": True, - }, - ) - def test_from_dict(self): data = { "metadata": {"test": "test"}, @@ -447,7 +328,6 @@ class TestPipeline: pipe = Pipeline.from_dict(data) assert pipe.metadata == {"test": "test"} - assert pipe.max_loops_allowed == 101 assert pipe._max_runs_per_component == 101 # Components @@ -508,7 +388,6 @@ class TestPipeline: def test_from_dict_with_callbacks(self): data = { "metadata": {"test": "test"}, - "max_loops_allowed": 101, "components": { "add_two": { "type": "haystack.testing.sample_components.add_value.AddFixedValue", @@ -604,7 +483,6 @@ class TestPipeline: components = {"add_two": add_two, "add_default": add_default} data = { "metadata": {"test": "test"}, - "max_loops_allowed": 100, "components": { "add_two": {}, "add_default": {}, @@ -617,7 +495,6 @@ class TestPipeline: } pipe = Pipeline.from_dict(data, components=components) assert pipe.metadata == {"test": "test"} - assert pipe.max_loops_allowed == 100 # Components assert len(pipe.graph.nodes) == 3 @@ -678,7 +555,6 @@ class TestPipeline: def test_from_dict_without_component_type(self): data = { "metadata": {"test": "test"}, - "max_loops_allowed": 100, "components": {"add_two": {"init_parameters": {"add": 2}}}, "connections": [], } @@ -691,7 +567,6 @@ class TestPipeline: def test_from_dict_without_registered_component_type(self, request): data = { "metadata": {"test": "test"}, - "max_loops_allowed": 100, "components": {"add_two": {"type": "foo.bar.baz", "init_parameters": {"add": 2}}}, "connections": [], } @@ -702,12 +577,7 @@ class TestPipeline: # UNIT def test_from_dict_without_connection_sender(self): - data = { - "metadata": {"test": "test"}, - "max_loops_allowed": 100, - "components": {}, - "connections": [{"receiver": "some.receiver"}], - } + data = {"metadata": {"test": "test"}, "components": {}, "connections": [{"receiver": "some.receiver"}]} with pytest.raises(PipelineError) as err: Pipeline.from_dict(data) @@ -715,12 +585,7 @@ class TestPipeline: # UNIT def test_from_dict_without_connection_receiver(self): - data = { - "metadata": {"test": "test"}, - "max_loops_allowed": 100, - "components": {}, - "connections": [{"sender": "some.sender"}], - } + data = {"metadata": {"test": "test"}, "components": {}, "connections": [{"sender": "some.sender"}]} with pytest.raises(PipelineError) as err: Pipeline.from_dict(data) diff --git a/test/core/pipeline/test_tracing.py b/test/core/pipeline/test_tracing.py index 73cbe55de..83294bcc2 100644 --- a/test/core/pipeline/test_tracing.py +++ b/test/core/pipeline/test_tracing.py @@ -45,7 +45,6 @@ class TestTracing: "haystack.pipeline.input_data": {"hello": {"word": "world"}}, "haystack.pipeline.output_data": {"hello2": {"output": "Hello, Hello, world!!"}}, "haystack.pipeline.metadata": {}, - "haystack.pipeline.max_loops_allowed": 100, "haystack.pipeline.max_runs_per_component": 100, }, trace_id=ANY, @@ -100,7 +99,6 @@ class TestTracing: operation_name="haystack.pipeline.run", tags={ "haystack.pipeline.metadata": {}, - "haystack.pipeline.max_loops_allowed": 100, "haystack.pipeline.max_runs_per_component": 100, "haystack.pipeline.input_data": {"hello": {"word": "world"}}, "haystack.pipeline.output_data": {"hello2": {"output": "Hello, Hello, world!!"}}, diff --git a/test/core/test_serialization.py b/test/core/test_serialization.py index 755271bde..d6a3980bf 100644 --- a/test/core/test_serialization.py +++ b/test/core/test_serialization.py @@ -64,7 +64,6 @@ def test_default_component_from_dict_unregistered_component(request): def test_from_dict_import_type(): pipeline_dict = { "metadata": {}, - "max_loops_allowed": 100, "components": { "greeter": { "type": "haystack.testing.sample_components.greet.Greet", diff --git a/test/test_files/yaml/test_pipeline_deprecated.yaml b/test/test_files/yaml/test_pipeline_deprecated.yaml deleted file mode 100644 index ee8b3f689..000000000 --- a/test/test_files/yaml/test_pipeline_deprecated.yaml +++ /dev/null @@ -1,14 +0,0 @@ -components: - Comp1: - init_parameters: - an_init_param: null - type: test.core.pipeline.test_pipeline.FakeComponent - Comp2: - init_parameters: - an_init_param: null - type: test.core.pipeline.test_pipeline.FakeComponent -connections: -- receiver: Comp2.input_ - sender: Comp1.value -max_loops_allowed: 99 -metadata: {}