Suggestion to Refactor Flask Endpoint to Avoid Using `subprocess`

[K-dash]

I was going to start working on the implementation of the server.py test code, but there were some areas where I thought the process could be improved, so I made this issue!

The current implementation of the Flask endpoint in app.py uses a subprocess to execute the run function from cli.py. I think this approach introduces extra overhead and complexity.

Current Code

@app.route("/")
def default_route():
... snip ...
@app.route("/")
def default_route():
    query_parameters = urllib.parse.parse_qsl(
        request.query_string.decode(), keep_blank_values=True
    )
    parsed_parameters = []

    for key, value in query_parameters:
        if value:
            parsed_parameters.append(f"{key}={value}")
        else:
            parsed_parameters.append(key)

    args = ",".join(parsed_parameters)

    async def run_subprocess():
        try:
            result = subprocess.run(
                ["python3", "src/cli.py", args],
                capture_output=True,
                text=True,
                check=True,
            )
            return result.stdout
        except subprocess.CalledProcessError as e:
            print("Error message from subprocess:", e.stderr)
            raise e

    loop = asyncio.new_event_loop()
    asyncio.set_event_loop(loop)
    result = loop.run_until_complete(run_subprocess())
    return result

Suggestion

How about refactoring the code to import and call the run function directly from cli.py and eliminate the need for sub-processes? An image of the proposed amendment is below. (although error handling, etc. will need to be considered separately)

from src.cli import run  # Direct import

@app.route("/")
def default_route():
    query_parameters = urllib.parse.parse_qsl(
        request.query_string.decode(), keep_blank_values=True
    )
    parsed_parameters = []

    for key, value in query_parameters:
        if value:
            parsed_parameters.append(f"{key}={value}")
        else:
            parsed_parameters.append(key)

    args = ",".join(parsed_parameters)
    result = run(args)  # Direct function call
    return result

Of course, the cli.py side must also be modified to make this modification.

I think the advantages of calling cli.py directly are as follows.

• Reduces overhead and complexity

  • Using subprocess adds extra layers of process creation and inter-process communication. Directly calling the function within the same process simplifies the code and avoids these unnecessary steps.

• Simplifies error handling

  • When using sub-processes, cli.py is executed by the new process, which makes it difficult to catch errors when they occur. cli.pycan be called directly to easily locate the error (easily tracked by the IDE's debugger, etc.) (which can be easily tracked using the IDE's debugger, etc.).

@ryansurf However, I am wrong and may have missed something. If you are using Subprocess by design, please tell me why!

[K-dash]

If you change the code as I suggested, the output from cli.py would be the standard output of the Flask server, and the results would not be displayed in the terminal of the user who made the curl request. (This is because cli.py uses print statements to output the final ASCII art and the results obtained from the API.)

Also, if the concept of this tool is to be used from the CLI, then perhaps a sub-process implementation is still appropriate. My apologies. I found out a lot and solved the problem myself. Please feel free to close this Issue.

[ryansurf]

Indeed, the intention is to have it be mostly used from the CLI (although there is the frontend, as bad as it may be atm).

Before I saw this I was briefly trying to work on the server testing code, and I'm not sure how to move forward (to start the server we'd need to implement threading/subprocess to test it I think?).

I like your above implementation, it looks clean. I wonder if theres a way we can solve the terminal issue 🤔

Regardless, thanks for the writeup. I think we should improve the Flask code one way or another, its a little messy

[K-dash]

Before I saw this I was briefly trying to work on the server testing code, and I'm not sure how to move forward (to start the server we'd need to implement threading/subprocess to test it I think?).

Flask provides a Client feature for testing purposes, which we will use. With this, it is possible to run tests without starting the Flask server on the developer side:)

https://flask.palletsprojects.com/en/3.0.x/testing/#sending-requests-with-the-test-client

Sending Requests with the Test Client The test client makes requests to the application without running a live server.

[K-dash]

I like your above implementation, it looks clean. I wonder if theres a way we can solve the terminal issue 🤔

I'm currently using a print statement in cli.py, which is called from subprocess, to output the formatted result. there's a proposal to change this so that the result is returned to the user who executed the curl command as an "HTTP response".

However, Please note that adopting this change would require significant refactoring. Therefore, I think the decision to adopt it should be carefully considered based on the future benefits and the usability for future users (e.g. whether it leans more towards CLI operations or frontend usage). 🤔

[K-dash]

I was just thinking that if we focus on the frontend (reflecting the output results on the frontend), we could represent various ocean data in graphs, which would open up more possibilities. Although it doesn't quite align with the concept of cli-surf, haha. 😓

[ryansurf]

I was just thinking that if we focus on the frontend (reflecting the output results on the frontend), we could represent various ocean data in graphs, which would open up more possibilities. Although it doesn't quite align with the concept of cli-surf, haha. 😓

I agree. When I first started this project I was thinking only focusing on the cli, but why not a frontend? :joy:

I was actually trying to plot a forecast of the surf heights over a 7 day period using JSCharting and had it working, but I scrapped it (wasn't happy with how it looked). I think we should give it another go, altough my frontend skills are quite lacking lol

[ryansurf]

I like your above implementation, it looks clean. I wonder if theres a way we can solve the terminal issue 🤔

I'm currently using a print statement in cli.py, which is called from subprocess, to output the formatted result. there's a proposal to change this so that the result is returned to the user who executed the curl command as an "HTTP response".

However, Please note that adopting this change would require significant refactoring. Therefore, I think the decision to adopt it should be carefully considered based on the future benefits and the usability for future users (e.g. whether it leans more towards CLI operations or frontend usage). 🤔

I think this would be a good change. I'm going to look into this today, seems like it shouldn't be too much work... but those are some famous last words

[K-dash]

I was actually trying to plot a forecast of the surf heights over a 7 day period using JSCharting and had it working, but I scrapped it (wasn't happy with how it looked). I think we should give it another go, altough my frontend skills are quite lacking lol

That's a great initiative! I'm familiar with ChartJS, but I wasn't aware of JSCharting. Thank you. As for the frontend, I don't have much knowledge either and am currently learning about it. 😇

[K-dash]

@ryansurf Are you familiar with Streamlit?

https://streamlit.io/ https://streamlit.io/gallery

With Streamlit, you can implement a frontend using only Python code. It allows for easy data visualization and dashboard creation, making it suitable for various data science use cases. So, it might be worth considering adopting it. However, since Streamlit is ultimately a Python-based framework, if complex UI becomes necessary, I believe it would be necessary to adopt a frontend framework.

[ryansurf]

@ryansurf Are you familiar with Streamlit?

https://streamlit.io/ https://streamlit.io/gallery

With Streamlit, you can implement a frontend using only Python code. It allows for easy data visualization and dashboard creation, making it suitable for various data science use cases. So, it might be worth considering adopting it. However, since Streamlit is ultimately a Python-based framework, if complex UI becomes necessary, I believe it would be necessary to adopt a frontend framework.

This is cool!

Hmm, thats what im debating at the moment. We could have a simple frontend that uses streamlit. We can also implement a more complex frontend that lets users login, save surf spots, etc. Issue is I am not a big frontend person :joy:

I think we should use this for now