Repository: explosion/spaCy
Pull request: Revert disable/disabled merging behavior
## Description
<!--- Use this section to describe your changes. If your changes required
testing, include information about the testing environment and the tests you
ran. If your test fixes a bug reported in an issue, don't forget to include the
issue number. If your PR is still a work in progress, that's totally fine – just
include a note to let us know. -->
Resolves https://github.com/explosion/spaCy/issues/11706 and related issues. This modifies/reverts the merging behavior for `disable`/`disabled`, so that both sets of pipe names are merged instead of the latter being overwritten by the former (which is still the active behavior for `enable`/`enabled`). 

Adjusts tests, warnings and errors.

### Types of change
<!-- What type of change does your PR cover? Is it a bug fix, an enhancement
or new feature, or a change to the documentation? -->
Bug fix? We revert to pre-v3.4.2 behavior.

## Checklist
<!--- Before you submit the PR, go over this checklist and make sure you can
tick off all the boxes. [] -> [x] -->
- [x] I confirm that I have the right to submit this contribution under the project's MIT license.
- [x] I ran the tests, and all new and existing tests passed.
- [x] My changes don't require a change to the documentation, or if they do, I've added all required information.


Problem statement:
UserWarning W123 printed on console after upgrading to 3.4.2
After upgrading to `3.4.2` I see this warning on the console when a model is loaded. How to fix or silence this?

> spacy/language.py:1895: UserWarning: [W123] Argument disable with value [] is used instead of ['senter'] as specified in the config. Be aware that this might affect other components in your pipeline.

## How to reproduce the behaviour
```python
import en_core_web_sm
lang_model = en_core_web_sm.load()
```

## Your Environment

- **spaCy version:** 3.4.2
- **Platform:** macOS-13.0-arm64-arm-64bit
- **Python version:** 3.10.6
- **Pipelines:** en_core_web_md (3.4.1), en_core_web_sm (3.4.1), en_core_web_lg (3.4.1)

Hints from issue discussion:
Thanks for the heads-up, I confirmed this behavior. It's probably related to #11459. 

This only happens for me when using the `en_core_web_sm.load()` style of loading - there's no warning with `spacy.load()`. 

We'll take a look at this, but it should be safe to ignore if the behavior it warns about is what you want. You can silence this the same was as any Python warning, like so:

```
import warnings
# message is a prefix of the actual warning string
warnings.filterwarnings("ignore", message="[W123]")
```
When disabling a component with 
`nlp = spacy.load('en_core_web_sm', disable=['ner'])`
the same error is triggered. Is this issue related?

I tried the fix suggest above with:
```import spacy
import warnings

warnings.filterwarnings("ignore", message="[W123]")

nlp = spacy.load('en_core_web_sm', disable=['ner'])
```

but it did not work. 
I am still getting this warning:


```/somepath/test/spacy/load_model_error_W123.py 
/Users/x/environments/python3.10.8/lib/python3.10/site-packages/spacy/language.py:1895: UserWarning: [W123] Argument disable with value ['ner'] is used instead of ['senter'] as specified in the config. Be aware that this might affect other components in your pipeline.
  warnings.warn(
```

UPDATE: warnings.filterwarnings("ignore", message="[W123]") needs to be 

```
warnings.filterwarnings("ignore", message="^\[W123\]")
```

as message filter uses regex!

Any hint on how to correctly load the model without triggering the warning?
Thx!
#11713 fixed the warning for `en_core_web_sm.load()` but there are still some bugs related to the behavior of `en_core_web_sm.load(disable=["ner"])` in v3.4.2.
A workaround to get the intended behavior for the `sm/md/lg` pipelines in v3.4.2 is to load the model like this:

```python
nlp = spacy.load('en_core_web_sm', disable=['ner', 'senter'])
```

This will still produce a warning, though, and we are working on fixing both the behavior for `disable=['ner']` and improving the warnings for cases that still need a warning.

If the warning is still causing problems for you and you don't need any of the modifications in v3.4.2, you can consider temporarily downgrading to v3.4.1.
To explain a bit more for anyone who comes across this issue, in v3.4.2 `spacy.load("en_core_web_sm", disable=["ner"])` is doing two things:

1. disables `ner`
1. enables `senter`, which is otherwise disabled by default

In spacy v3.4.1 and earlier, the same command only did (1) and not (2). Enabling `senter` as a side effect is a bug, and we are working on addressing this for v3.4.3.

Commit message: Merge disable with disabled. Adjust warnings, errors and tests.

Patch to review:
diff --git a/spacy/errors.py b/spacy/errors.py
index e0628819dc9..5e17053cf70 100644
--- a/spacy/errors.py
+++ b/spacy/errors.py
@@ -212,8 +212,8 @@ class Warnings(metaclass=ErrorsWithCodes):
     W121 = ("Attempting to trace non-existent method '{method}' in pipe '{pipe}'")
     W122 = ("Couldn't trace method '{method}' in pipe '{pipe}'. This can happen if the pipe class "
             "is a Cython extension type.")
-    W123 = ("Argument {arg} with value {arg_value} is used instead of {config_value} as specified in the config. Be "
-            "aware that this might affect other components in your pipeline.")
+    W123 = ("Argument `enable` with value {enable} does not contain all values specified in the config option "
+            "`enabled` ({enabled}). Be aware that this might affect other components in your pipeline.")
 
 
 class Errors(metaclass=ErrorsWithCodes):
diff --git a/spacy/language.py b/spacy/language.py
index d391f15aba1..e83bd1886f5 100644
--- a/spacy/language.py
+++ b/spacy/language.py
@@ -1879,31 +1879,22 @@ def from_config(
         if isinstance(exclude, str):
             exclude = [exclude]
 
-        def fetch_pipes_status(value: Iterable[str], key: str) -> Iterable[str]:
-            """Fetch value for `enable` or `disable` w.r.t. the specified config and passed arguments passed to
-            .load(). If both arguments and config specified values for this field, the passed arguments take precedence
-            and a warning is printed.
-            value (Iterable[str]): Passed value for `enable` or `disable`.
-            key (str): Key for field in config (either "enabled" or "disabled").
-            RETURN (Iterable[str]):
-            """
-            # We assume that no argument was passed if the value is the specified default value.
-            if id(value) == id(_DEFAULT_EMPTY_PIPES):
-                return config["nlp"].get(key, [])
-            else:
-                if len(config["nlp"].get(key, [])):
-                    warnings.warn(
-                        Warnings.W123.format(
-                            arg=key[:-1],
-                            arg_value=value,
-                            config_value=config["nlp"][key],
-                        )
+        # `enable` should not be merged with `enabled` (the opposite is true for `disable`/`disabled`). If the config
+        # specifies values for `enabled` not included in `enable`, emit warning.
+        if id(enable) != id(_DEFAULT_EMPTY_PIPES):
+            enabled = config["nlp"].get("enabled", [])
+            if len(enabled) and not set(enabled).issubset(enable):
+                warnings.warn(
+                    Warnings.W123.format(
+                        enable=enable,
+                        enabled=enabled,
                     )
-                return value
+                )
 
+        # Ensure sets of disabled/enabled pipe names are not contradictory.
         disabled_pipes = cls._resolve_component_status(
-            fetch_pipes_status(disable, "disabled"),
-            fetch_pipes_status(enable, "enabled"),
+            list({*disable, *config["nlp"].get("disabled", [])}),
+            enable,
             config["nlp"]["pipeline"],
         )
         nlp._disabled = set(p for p in disabled_pipes if p not in exclude)
@@ -2084,10 +2075,12 @@ def _resolve_component_status(
         if enable:
             if isinstance(enable, str):
                 enable = [enable]
-            to_disable = [
-                pipe_name for pipe_name in pipe_names if pipe_name not in enable
-            ]
-            if disable and disable != to_disable:
+            to_disable = {
+                *[pipe_name for pipe_name in pipe_names if pipe_name not in enable],
+                *disable,
+            }
+            # If any pipe to be enabled is in to_disable, the specification is inconsistent.
+            if any([pipe_to_enable in to_disable for pipe_to_enable in enable]):
                 raise ValueError(Errors.E1042.format(enable=enable, disable=disable))
 
         return tuple(to_disable)
diff --git a/spacy/tests/pipeline/test_pipe_methods.py b/spacy/tests/pipeline/test_pipe_methods.py
index 14a7a36e55d..394a4820b10 100644
--- a/spacy/tests/pipeline/test_pipe_methods.py
+++ b/spacy/tests/pipeline/test_pipe_methods.py
@@ -615,20 +615,18 @@ def test_enable_disable_conflict_with_config():
 
     with make_tempdir() as tmp_dir:
         nlp.to_disk(tmp_dir)
-        # Expected to fail, as config and arguments conflict.
-        with pytest.raises(ValueError):
-            spacy.load(
-                tmp_dir, enable=["tagger"], config={"nlp": {"disabled": ["senter"]}}
-            )
+        # Expected to succeed, as config and arguments conflict.
+        assert spacy.load(
+            tmp_dir, enable=["tagger"], config={"nlp": {"disabled": ["senter"]}}
+        ).disabled == ["senter", "sentencizer"]
         # Expected to succeed without warning due to the lack of a conflicting config option.
         spacy.load(tmp_dir, enable=["tagger"])
-        # Expected to succeed with a warning, as disable=[] should override the config setting.
-        with pytest.warns(UserWarning):
+        # Expected to fail due to conflict between enable and disabled.
+        with pytest.raises(ValueError):
             spacy.load(
                 tmp_dir,
-                enable=["tagger"],
-                disable=[],
-                config={"nlp": {"disabled": ["senter"]}},
+                enable=["senter"],
+                config={"nlp": {"disabled": ["senter", "tagger"]}},
             )
 
 
diff --git a/spacy/tests/serialize/test_serialize_pipeline.py b/spacy/tests/serialize/test_serialize_pipeline.py
index b948bb76c00..9fcf18e2d12 100644
--- a/spacy/tests/serialize/test_serialize_pipeline.py
+++ b/spacy/tests/serialize/test_serialize_pipeline.py
@@ -404,11 +404,10 @@ def test_serialize_pipeline_disable_enable():
     assert nlp3.component_names == ["ner", "tagger"]
     with make_tempdir() as d:
         nlp3.to_disk(d)
-        with pytest.warns(UserWarning):
-            nlp4 = spacy.load(d, disable=["ner"])
-    assert nlp4.pipe_names == ["tagger"]
+        nlp4 = spacy.load(d, disable=["ner"])
+    assert nlp4.pipe_names == []
     assert nlp4.component_names == ["ner", "tagger"]
-    assert nlp4.disabled == ["ner"]
+    assert nlp4.disabled == ["ner", "tagger"]
     with make_tempdir() as d:
         nlp.to_disk(d)
         nlp5 = spacy.load(d, exclude=["tagger"])


Code review comments:
diff_hunk: @@ -615,20 +615,18 @@ def test_enable_disable_conflict_with_config():
 
     with make_tempdir() as tmp_dir:
         nlp.to_disk(tmp_dir)
-        # Expected to fail, as config and arguments conflict.
-        with pytest.raises(ValueError):
-            spacy.load(
-                tmp_dir, enable=["tagger"], config={"nlp": {"disabled": ["senter"]}}
-            )
+        # Expected to succeed, as config and arguments conflict.
path: spacy/tests/pipeline/test_pipe_methods.py
text: @user1:
?

```suggestion
        # Expected to succeed, as config and arguments do not conflict.
```

diff_hunk: @@ -2084,10 +2075,12 @@ def _resolve_component_status(
         if enable:
             if isinstance(enable, str):
                 enable = [enable]
-            to_disable = [
-                pipe_name for pipe_name in pipe_names if pipe_name not in enable
-            ]
-            if disable and disable != to_disable:
+            to_disable = {
+                *[pipe_name for pipe_name in pipe_names if pipe_name not in enable],
+                *disable,
+            }
+            # If any pipe to be enabled is in to_disable, the specification is inconsistent.
+            if any([pipe_to_enable in to_disable for pipe_to_enable in enable]):
path: spacy/language.py
text: @user1:
I was surprised that this check didn't use a set operation?

@author:
No particular reason for this. Replaced with a set overlap check.

@user1:
I was just surprised because I initially thought something more complicated must be going on, but it wasn't, right?

@author:
Before this PR we [checked for equivalence](https://github.com/explosion/spaCy/pull/11745/files#diff-fe971afb0a4f4d99848a30cfc2708c359e30703084ea5220629928ac9015f15dL2090) between the sets of pipes defined via `exclude` and those via `include` (and raised an error in case of inequality).

With our current assumptions, I think a conflict should occur if and only if the sets of pipes to be enabled (in `enable(d)`) and pipes to be disabled (pipes not in `enable(d)` + pipes in `disable(d)`). Do you agree?

@user1:
The current check is what I expected.