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

Improvement: Backend Improvements for Typing, Tests, Modularity and Pathlib #34

Open
omeryusufyagci opened this issue Oct 7, 2024 · 6 comments
Assignees
Labels
backend good first issue Good for newcomers hacktoberfest This issue is eligible for Hacktoberfest contributions help wanted Extra attention is needed improvement Improve existing functionality refactor urgent
Milestone

Comments

@omeryusufyagci
Copy link
Owner

Current app.py will be reorganized under backend/, with the following objectives:

  • Typing: Add type hints throughout the backend
  • Unit Tests: Develop unit tests for all utility and processing functions.
  • Response Handling: Add a ResponseHandler class in backend/response_handler.py to handle all communication bwn BE-FE, BE-core using consistent JSON responses.
  • Replace os with pathlib
  • Reorganize the code under backend/:
    • Move Utils to backend/utils.py
    • Move MediaHandler to backend/media_handler.py
    • Keep app.py for route definitions and Flask app initialization.

This is a big step going towards v1.0 and offers a relatively straightforward first issue for contributors looking to make an impactful contribution to the project.

@omeryusufyagci omeryusufyagci added help wanted Extra attention is needed good first issue Good for newcomers improvement Improve existing functionality refactor backend labels Oct 7, 2024
@omeryusufyagci omeryusufyagci added this to the v1.0 milestone Oct 7, 2024
@omeryusufyagci omeryusufyagci changed the title Improvement: Backend Refactor for Typing, Tests, Modularity and Pathlib Improvement: Backend Improvements for Typing, Tests, Modularity and Pathlib Oct 8, 2024
@omeryusufyagci omeryusufyagci added hacktoberfest This issue is eligible for Hacktoberfest contributions urgent labels Oct 8, 2024
@omeryusufyagci omeryusufyagci pinned this issue Oct 15, 2024
Repository owner deleted a comment from wenayy Oct 15, 2024
Repository owner deleted a comment from wenayy Oct 15, 2024
@prakash-2002-ramanathan

Is this issue open? Shall I open a PR?

@omeryusufyagci
Copy link
Owner Author

Hi @prakash-2002-ramanathan, thanks for your interest. Indeed, it's open and much needed!

Would be great if you could open a PR!

@prakash-2002-ramanathan

Hey, I have done all of the work, but not quite sure what you mean by "make the Backend to Core communication Consistent JSON "?
Like You want to start a server in C++ to handle JSON responses or pass the data in JSON format to the Media Handler subprocess ?

@omeryusufyagci
Copy link
Owner Author

Thanks for the question @prakash-2002-ramanathan,

We should ensure consistency across the application, using json responses between all layers. Currently the MediaProcessor writes to stdout and the backend is parsing directly these traces. Instead, the core should return a json object.

Likewise, anything between the frontend and backend should be managed through json objects for consistency.

Does this answer your question?

@prakash-2002-ramanathan
Copy link

prakash-2002-ramanathan commented Oct 22, 2024

I Appreciate your response. But how do I implement this. Do I pass the a JSON object as a string in the Std::cout from MediaHandler, and parse the JSON from result.stdout in python? Or is there a better way to communicate between the BE and the Core ? Any suggestions for improving this communication pattern?

I will give the input to the MediaHandler in subprocess. And shall I make it return the JSON output using HTTP to the sever?

@omeryusufyagci
Copy link
Owner Author

Well, we have an open issue to add a proper logger, e.g. spdlog. Until that comes, we're still writing plain text to cout/cerr.

In the future we'll probably use gRPC for inter-process communication (IPC), but at this stage of the project I'd rather get something significantly less error prone working first. In that sense, for now, we can dedicate cerr for logging, leaving cout isolated for our IPC.

Let's make sure the json structure stays consistent, so that we can write generic parsers.
I'll suggest something like:

{
    "type": "progress",
    "payload": {
        "status": "in_progress",
        "message": "Processing audio",
        "progress": {
            "percentage": 75
        }
    }
}

and for a response type we'd omit the progress. The parser should expect the absence of certain parameters and default to a sensible value.

{
    "type": "response",
    "payload": {
        "status": "success",
        "message": "Processing completed",
    }
}

Currently responses are more important than the progress packets, but still we should think ahead as we implement this.

Please let me know if you have any other questions or need help during the implementation. Thanks!

prakash-2002-ramanathan added a commit to prakash-2002-ramanathan/fast-music-remover that referenced this issue Oct 31, 2024
*(deletion) Original app.py

*(Updation) requirements

*(modification) make changes to preserve the CLI nature and only make
the backend changes
@omeryusufyagci omeryusufyagci unpinned this issue Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend good first issue Good for newcomers hacktoberfest This issue is eligible for Hacktoberfest contributions help wanted Extra attention is needed improvement Improve existing functionality refactor urgent
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants