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

Respect returned synapse (and its status code) #1822

Conversation

mjurbanski-reef
Copy link
Contributor

@mjurbanski-reef mjurbanski-reef commented Apr 29, 2024

This continues from #1803 by adding fix for bittensor ignoring synapse objects returned by forward_fn. Most painful effect of that was that while user may have been convinced synapse.axon.status_code will be propagated to the client, it in fact, was not.

I have noticed that @ifrit98 was also hit by this bug at some point:
https://github.com/ifrit98/storage-subnet/blob/b92052e0c33aea0d3d04ce707e183964e442c1b5/neurons/miner.py#L776-L780

Fixes&improvements

  • forward_fn return synapse is no longer ignored - before, if we for example did synapse = synapse.copy() at the beginning of that function and modified only a copy, you could spent hours no end without seeing why your axon doesn't actually return that to the client (dendrite); don't ask me how I know :)
  • synapse.axon.processing_time is now also sent to the end client (before it was only visible on the server)
  • added some tests testing the axon-fastapi interface proving that the proposed change does work (they use fastapi mock server)
  • SynapseException risen from forward_fn allow to set custom error and status codes
  • other internal exception are no longer shown to the remote client as they could disclose confidential information (same thing as

Overall this should improve debugging experience a lot. It sure did for me.

@gus-opentensor
Copy link
Collaborator

Thanks @mjurbanski-reef ! we will take a look

@mjurbanski-reef mjurbanski-reef force-pushed the respect_returned_synapse_status_code branch from d2a36ea to bb6228f Compare May 15, 2024 08:22
@mjurbanski-reef
Copy link
Contributor Author

mjurbanski-reef commented May 15, 2024

@gus-opentensor I have rebased to current staging. IMO it would be good to include in v7.0 since it is a substantial change, also it hangs here for 2 weeks already and it would be a shame to let it waste away ;)

Since I had some time to play with it, I additionally, fixed the exception handling, so now if SynapseException is risen from forward_fn it will respect its value as well, but perhaps more importantly, it no longer will return exception as "raw" to the client.
This was a potential security issue as for example

yaml.safe_load("{password=SuperConfidentialPassword")

if triggered under forward_fn would happily return to the remote client the 'while parsing a flow mapping\n in "<unicode string>", line 1, column 1:\n {password=SuperConfidentialPassword\n ^\nexpected \',\' or \'}\', but got \'<stream end>\'\n in "<unicode string>", line 1, column 36:\n {password=SuperConfidentialPassword\n ^' error message containing the password.

@mjurbanski-reef
Copy link
Contributor Author

Seems on dendrite side the error message was ignored as well, so fixed that as well and added some more tests

@mjurbanski-reef
Copy link
Contributor Author

another cool application possible after these changes is using BackgroundTasks from fastapi https://fastapi.tiangolo.com/tutorial/background-tasks/ in forward_fn

@mjurbanski-reef
Copy link
Contributor Author

@gus-opentensor so this PR is 3 weeks old, any hope for it to be included in v7?

@gus-opentensor gus-opentensor merged commit bd4bf0b into opentensor:staging May 22, 2024
11 checks passed
@mjurbanski-reef
Copy link
Contributor Author

@gus-opentensor thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants