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

Roman/dry ingest pipeline step #3203

Merged
merged 6 commits into from
Jun 14, 2024
Merged

Conversation

rbiseck3
Copy link
Contributor

Description

The main goal of this was to reduce the duplicate code that was being written for each ingest pipeline step to support async and not async functionality.

Additional bug fixes found and fixed:

  • each logger for ingest wasn't being instantiated correctly. This was fixed to instantiate in the beginning of a pipeline run as soon as the verbosity level can be determined.
  • The requires_dependencies wrapper wasn't wrapping async functions correctly. This was fixed so that asyncio.iscoroutinefunction() gets trigger correctly.

@@ -61,7 +61,7 @@ def _wrap_mp(self, input_kwargs: dict) -> Any:

def _set_log_level(self, log_level: int):
# Set the log level for each spawned process when using multiprocessing pool
logger.setLevel(log_level)
make_default_logger(log_level)
Copy link
Contributor

@potter-potter potter-potter Jun 13, 2024

Choose a reason for hiding this comment

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

This causes logs to be created for every process I think. In other words, I see 2 of the same log for most things.

Though I think it is worth it if it may surface errors better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this in the latest commit

Copy link
Contributor

@potter-potter potter-potter left a comment

Choose a reason for hiding this comment

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

looks good. tqdm is a bonus

@rbiseck3 rbiseck3 force-pushed the roman/dry-ingest-pipeline-step branch from 4cf96ac to 85e374b Compare June 14, 2024 13:00
@rbiseck3 rbiseck3 added this pull request to the merge queue Jun 14, 2024
Merged via the queue into main with commit a6c09ec Jun 14, 2024
46 checks passed
@rbiseck3 rbiseck3 deleted the roman/dry-ingest-pipeline-step branch June 14, 2024 14:27
Copy link

sentry-io bot commented Jun 17, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ IndexError: list index out of range /general/v0/general View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants