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

Split feedback_pipeline into multiple files and added a config manager #77

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Fan
Copy link
Contributor

@Fan Fan commented Nov 20, 2024

Splitting up the feedback_pipeline into multiple files.

I kept the current feedback_pipeline.py file as the main entry point for the program. The new files will probably undergo a lot more changes as we implement other changes and features. This is probably a good start to a bigger refactor.

Copy link
Collaborator

@yselkowitz yselkowitz left a comment

Choose a reason for hiding this comment

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

For starters, the imports in the test_*.py files need to be updated for this restructuring; that's why CI is currently failing.

I'm not quite sure what the consequences of having a feedback_pipeline.py script and a feedback_pipeine/__init__.py in the same directory might be (wrt import feedback_pipeline), although that should only affect the tests (we're not designing this as a library).

As for the contents of the restructuring, this will take longer to review, but let's start with fixing the CI.

@yselkowitz yselkowitz self-assigned this Nov 22, 2024
@yselkowitz
Copy link
Collaborator

Testing this locally.

Copy link
Collaborator

@yselkowitz yselkowitz left a comment

Choose a reason for hiding this comment

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

So far this looks fine in local testing, but as it will require manual intervention on the running instance, let's hold off on merging it until we're both around.

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