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

HTTPX Timeout setting not synced w/ Orthanc setting #24

Closed
jrkerns opened this issue Aug 15, 2023 · 5 comments
Closed

HTTPX Timeout setting not synced w/ Orthanc setting #24

jrkerns opened this issue Aug 15, 2023 · 5 comments

Comments

@jrkerns
Copy link

jrkerns commented Aug 15, 2023

I'm not sure how many calls this might apply to, but when calling a C-MOVE from a remote modality, the timeout setting in the data dict appears to be just for Orthanc. HTTPX has it's own timeout default of 5s, so if the requested timeout is larger than that and the request does not resolve in 5s, an HTTPX timeout occurs. Using the C-MOVE in the readme as a start:

from pyorthanc import RemoteModality, Orthanc

orthanc = Orthanc('http://localhost:8042', 'username', 'password')

modality = RemoteModality(orthanc, 'modality')

# Query (C-Find) on modality
data = {'Level': 'Study', 'Query': {'PatientID': '*'}}
query_response = modality.query(data=data)

answer = modality.get_query_answers()[query_response['ID']]
print(answer)

# Retrieve (C-Move) results of query on a target modality (AET)
modality.move(query_response['ID'], {'Timeout': 300})   # <-here's the problem

I'm working on a VPN w/ a slow connection, so the move command takes a while.
Without modification, this will always raise an HTTPX timeout, regardless of the timeout setting (since HTTPX is not being passed this setting). It seems like a relatively easy fix; something like:

timeout = cmove_data.get("Timeout", 5)
dict(self.client.post_modalities_id_query(self.modality, json=data, timeout=timeout))

here:

return dict(self.client.post_modalities_id_query(self.modality, json=data))

You'd need to adjust post_modalities_id_query and self._post as well to handle the parameter, but this would at least make HTTPX use the same timeout as requested for Orthanc.

When I adjusted this manually by removing the timeout (by adding timeout=None) the C-MOVE was successful given enough time.

@jrkerns jrkerns changed the title Timeout setting not synced w/ Orthanc setting HTTPX Timeout setting not synced w/ Orthanc setting Aug 15, 2023
@gacou54
Copy link
Owner

gacou54 commented Aug 17, 2023

Hi @jrkerns ,

That's an excellent point, we should be able to change the timeout of a RemoteModality. I think it would be better if the Orthanc class had a timeout parameter, so every call made with the client instance would use the timeout. I actually plan to add all the httpx.Client parameters in the Orthanc.__init__() for the next release (hopefully next week). Then, this would work:

orthanc = Orthanc('url', 'username', 'password', timeout=300)
modality = RemoteModality(orthanc, 'mymodality')

# Now all orthanc and modality calls should have the 300 s timeout 

For now, this should do the trick since Orthanc inherited from httpx.Client:

orthanc = Orthanc('url', 'username', 'password')
orthanc.timeout = 300
modality = RemoteModality(orthanc, 'mymodality')

@jrkerns
Copy link
Author

jrkerns commented Aug 17, 2023

Thanks, that's definitely easy. I ended up overriding .post :

class MyOrthanc(Orthanc):

   def post(*args, **kwargs):
        return super().post(*args, **kwargs, timeout=300)

but I like your solution more.

@Kiechlus
Copy link

I have the same problem, need to set the httpx timout because the default is much too short. Hope this will be released soon. Thanks a lot!

@gacou54
Copy link
Owner

gacou54 commented Aug 21, 2023

Hi @Kiechlus,

PR #26 is ongoing, I hope to release the new pyorthanc version this week. In the meantime, this should work

orthanc = Orthanc('url', 'username', 'password')
orthanc.timeout = 300  # Set the timeout

# now use orthanc as you would ordinary
orthanc.get_patients()
...

Thank you for your interest to make pyorthanc better!

@gacou54
Copy link
Owner

gacou54 commented Aug 30, 2023

timeout added in #26

@gacou54 gacou54 closed this as completed Aug 30, 2023
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

No branches or pull requests

3 participants