fix support for litserve>0.2.4

[ali-alshaar7]

LitGPT CI fails with latest LitServe (>=0.2.4), this PR fixes that.

The real issue was that LitGPT was always failing with LitServe but the tests were flaky and they passed. Post 0.2.4, LitServe added the mechanism to check if the inference worker is alive before starting the uvicorn worker and it made the LitGPT CI fail.

[t-vi]

Do we expect the serving to work on macos? that seems to be failing.

[ali-alshaar7]

Do we expect the serving to work on macos? that seems to be failing.

Yeah, passes locally.

[aniketmaurya]

Love this @ali-alshaar7!! thank you so much 🚀

Also, can we set the version >=0.2.7? This way we will know if in case LitGPT stops working for a new version of LitServe again?

[Borda]

@aniketmaurya do you see any reason why it may hang/fail on Mac?

[aniketmaurya]

reran with debugging enabled @Borda! probably some issue with LitGPT setting up the devices. Ali mentioned that it works locally. will debug this in depth later today.

[aniketmaurya]

I get this error locally:

 File "/Users/aniket/Projects/github/litgpt/litgpt/config.py", line 149, in from_file
    return cls(**file_kwargs)
           ^^^^^^^^^^^^^^^^^^
TypeError: Config.__init__() got an unexpected keyword argument 'sliding_window_layer_placing'

investigating more cc @ali-alshaar7 @k223kim

[k223kim]

@aniketmaurya Did you pull the main branch? That's coming from my gemma PR

[t-vi]

Also, can we set the version >=0.2.7? This way we will know if in case LitGPT stops working for a new version of LitServe again?

Note that the useful CI relation is opposite to >= dependencies: If we want to declare a >= 0.2.7 (or whatever version should be the minimum) dependency on LitServe, LitServe should ideally gain a test that updates to it don't break LitGPT.
If we don't have that, it seems more prudent to limit LitGPT compat to what we know we work with and then bump the dependency in a PR (that will check compat in the CI tests).

[aniketmaurya]

right @t-vi, we should aim for a test to ensure this!

[Borda]

we should aim for a test to ensure this!

it did not help so let's revert it back and try to debug it...

[aniketmaurya]

seems like the server process is not terminated properly on macos - ERROR: [Errno 48] Address already in use

[ali-alshaar7]

seems like the server process is not terminated properly on macos - ERROR: [Errno 48] Address already in use

Hey, thanks for looking into this. I did add a kill_process_tree which seemed to resolve that on Ubuntu and windows. In any case, it shouldn't fail the test, since if there's already a server at that port, the test can just ping that and pass right?

[aniketmaurya]

I think uvicorn runs in the main process and rest of the FastAPI logic lies in the child processes so if the child processes die then it won't be reachable.

Maybe try using this logic from LitServe tests for running and stopping the tests?

[ali-alshaar7]

I think uvicorn runs in the main process and rest of the FastAPI logic lies in the child processes so if the child processes die then it won't be reachable.

Yeah, so if we kill all the children, we should free the port and be able to spin up a new server and so we won't have this issue right? that's what kill_process_tree does.

[aniketmaurya]

yup, you're right! theoretically that's how things should go.

[Borda]

@aniketmaurya would be nice if litServe has a function to properly terminate itself :)