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

Tests for CLI app - init config generates train-able config #12173

Merged
merged 16 commits into from
Jul 31, 2023

Conversation

pmbaumgartner
Copy link
Contributor

@pmbaumgartner pmbaumgartner commented Jan 24, 2023

Adds a CLI app test that the init config command generates configs that are usable by train for each possible component.

Created as draft to collect feedback on this approach.

Description

The test is parameterized by each component's name and a set of example data that is serialized to a DocBin within the test (so that it can be used by train). My intent with this structure is to avoid writing redundant code with a test for each component, which would really balloon the length of this file - but we can also take that more verbose approach as well well if we think that's better, I'm not totally convinced on the parameterized version myself. We'd probably want to do some custom formatting of the input parameters as well because they're also quite lengthy.

I marked the test as slow because some components take a while to train (specifically the parser). Locally this test ran in:

===================== 7 passed, 4618 deselected in 216.11s (0:03:36) =====================

Types of change

Tests

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

Copy link
Contributor

@kadarakos kadarakos left a comment

Choose a reason for hiding this comment

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

Really nice test! If I understand correctly, currently the test checks whether init-config generates a runnable config for a pipeline with a single component? If so, should it also check combinations of components?

@svlandeg svlandeg added enhancement Feature requests and improvements tests New, missing or incorrect tests labels Jan 25, 2023
@pmbaumgartner
Copy link
Contributor Author

Really nice test! If I understand correctly, currently the test checks whether init-config generates a runnable config for a pipeline with a single component? If so, should it also check combinations of components?

I think that's do-able, it'll explode the parameter space for this test though. Are there any sets of components that have interaction effects - i.e. it's not just adding a new section for component2 to the config, but changing both component1 and component2?

@kadarakos
Copy link
Contributor

Hmm, the first thing that came to my mind is the tok2vec and especially the listener architecture. What is the default behavior, if we have multiple components in the init-config should they have each their own tok2vec or should the pipeline have a single tok2vec listener?

@pmbaumgartner
Copy link
Contributor Author

Oh yeah, the tok2vec listener is a good obvious case I missed :)

I think that's probably worth setting up another test with these combinations, also because the example data we put into it will have to change so that all elements needed for the pipeline are annotated - right now I only provide attributes for each specific component.

@svlandeg svlandeg marked this pull request as ready for review June 1, 2023 15:41
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

This all looks good and should be ready to merge. I'll just run the slow tests once more on this PR (they succeed locally) and will merge when green.

@svlandeg
Copy link
Member

@explosion-bot please test_slow

@explosion-bot
Copy link
Collaborator

explosion-bot commented Jul 26, 2023

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/spacy-slow-tests/builds/427

@svlandeg svlandeg marked this pull request as draft July 31, 2023 09:41
@svlandeg
Copy link
Member

svlandeg commented Jul 31, 2023

The slow tests were failing because of an indirect relation with the caplog fixture used in an unrelated test.

Before the last commit on this PR, test_cli_app.py calls the CLI methods which do things like

util.logger.setLevel(logging.DEBUG if verbose else logging.INFO)

which means that logger levels are sometimes set to INFO even if they were DEBUG before. In test_ner_warns_no_lookups, we have

with caplog.at_level(logging.DEBUG):

BUT that won't actually do anything if the main logger is set to INFO. Because caplog gets information forwarded from the main logger, it's not independent. And so the combination of things means that there's a path in the tests execution where now the logging level is set to INFO and the NER test doesn't get the debug warning message.

To fix this, in the last commit we're setting the logger level only to DEBUG if the verbose flag is set explicitely, and leaving it as-is otherwise.

@svlandeg
Copy link
Member

@explosion-bot please test_slow

@explosion-bot
Copy link
Collaborator

explosion-bot commented Jul 31, 2023

🪁 Successfully triggered build on Buildkite

URL: https://buildkite.com/explosion-ai/spacy-slow-tests/builds/430

@svlandeg svlandeg marked this pull request as ready for review July 31, 2023 12:43
@svlandeg svlandeg merged commit a0a1956 into explosion:master Jul 31, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements tests New, missing or incorrect tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants