CHIMEFRB / workflow

🌊 Working in flowstate.
0 stars 0 forks source link

[FIX] workflow.http.buckets and workflow.http.results should have default baseurl based on active workspace #96

Closed chrisfandrade16 closed 1 month ago

chrisfandrade16 commented 1 month ago

I should be able to do something like this:

from workflow.http.buckets import Buckets
from workflow.http.results import Results
buckets_api = Buckets()
results_api = Results()

But this throws an error:

1 validation error for Workflow HTTP Client
baseurl
  Field required [type=missing, input_value={}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.8/v/missing

Because Buckets and Results inherit Client which has:

baseurl: Union[str, List[str]] = Field(
        ..., description="Base URLs to for the server"
    )

That's fine, baseurl should be required, and you can't really specify a default here since Client is general for any Workflow service. So instead, in Buckets and Results there should be a validator which reads DEFAULT_WORKSPACE_PATH and gets the baseurl from there.

Or do we want to discouage the use of Buckets and Results directly in favour of HTTPContext?

odarotto commented 1 month ago

So instead, in Buckets and Results there should be a validator which reads DEFAULT_WORKSPACE_PATH and gets the baseurl from there.

This is already done by the HTTPContext, the idea is to use it instead of individual clients, by default the HTTPContext initializes all the clients but you can specify the backends you want the context to use like:

In [1]: from workflow.http.context import HTTPContext

In [2]: http = HTTPContext(backends=["buckets", "results"])

In [3]: http.pipelines # None

In [4]: http.buckets # Was initialized on backends
Out[4]: Buckets(baseurl='http://localhost:8004', timeout=15.0, token=None, session=<requests.sessions.Session object at 0x0000028F5B23D540>)
chrisfandrade16 commented 1 month ago

Yeah, I gotcha. I think it's just an unfortunate breaking change. I suppose if you want to initialize Buckets (or whatever service directly), you'd have to just give the baseurl. I will close this.