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

Fixed hackathon issues #8891

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

madhavajay
Copy link
Collaborator

  • Unable to set Settings due to no interface that allows it
  • No ability for Mongo to do ignore_duplicates and updates
  • No passing of kwargs between set methods in the Settings service
  • Fixed issue with Jobs not working with data > 16MB in mongo because data was fetched from blob storage and then re-saved

Description

Please include a summary of the change, the motivation, and any additional context that will help others understand your PR. If it closes one or more open issues, please tag them as described here.

Affected Dependencies

List any dependencies that are required for this change.

How has this been tested?

  • Describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • List any relevant details for your test configuration.

Checklist

- Unable to set Settings due to no interface that allows it
- No ability for Mongo to do ignore_duplicates and updates
- No passing of kwargs between set methods in the Settings service
- Fixed issue with Jobs not working with data > 16MB in mongo
  because data was fetched from blob storage and then re-saved
@@ -291,14 +291,25 @@ def _set(
f"The fields that should be unique are {keys}."
)
else:
# TODO: h4x fix ?????? how is this supposed to work? This returns if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we were not throwing error because ignore duplicates is set to True, but we're not overwriting the data. +1 to override the data in case ignore duplicates is True during .set.

context=self.auth_context, action_object=new_action_object
)

# TODO: do this elsewhere, because here it breaks mongo by setting data back
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Ideally, we shoudn't have nested action objects, but even in the case if were to unwrap nested action object, and save it again to the Database, we should do the following:

data = action_object.syft_action_data
new_data = self.unwrap_nested_actionobjects(data)
new_action_object = ActionObject.from_obj(new_data, id=action_object.id)

# this will save the actual data to blob storage and only save the metadata in mongo store.
new_action_object._save_to_blob_storage()
res = self.action_service.set(
            context=self.auth_context, action_object=new_action_object
        )


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea was that you can do something like this

@syft_function
def map():
   ...
@syft_function
def reduce():
   ...

@syft_function
def outer():
   map_results=[]
   for x in [1,2,3]:
       map_job = domain.launch_job(map, x)
       map_results.append(map.job.results) # this does not exist yet, but we do have a pointer
   
   reduce_job = domain.launch_job(reduce, map_results) # this is a list of ActionObjects that do not exist yet
   return reduce_job.result

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed on how it should be stored @shubham3121. I think this was (partially?) created before we had blob storage so it was probably not changed when we introduced blob storage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants