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

Unable to subject.save simple metadata addition #220

Open
PmasonFF opened this issue Jul 4, 2019 · 11 comments
Open

Unable to subject.save simple metadata addition #220

PmasonFF opened this issue Jul 4, 2019 · 11 comments
Labels

Comments

@PmasonFF
Copy link

PmasonFF commented Jul 4, 2019

This simple metadata correction code is not working...

Panoptes.connect(username=os.environ['User_name'], password=os.environ['Password'])
subjects = Subject.where(subject_set_id=67143)
for subject in subjects:
    subject.metadata['Volume'] = '5'
    subject.save()

Traceback (most recent call last):
File "C:/py_scripts/Scripts_WoW/subject_update_metadata_from_file_simplified.py", line 10, in
subject.save()
File "C:\Users\User1\AppData\Local\Programs\Python\Python37-32\lib\site-packages\panoptes_client\subject.py", line 143, in save
log_args=False,
File "C:\Users\User1\AppData\Local\Programs\Python\Python37-32\lib\site-packages\redo_init_.py", line 185, in retry
return action(*args, **kwargs)
File "C:\Users\User1\AppData\Local\Programs\Python\Python37-32\lib\site-packages\panoptes_client\panoptes.py", line 815, in save
etag=self.etag
File "C:\Users\User1\AppData\Local\Programs\Python\Python37-32\lib\site-packages\panoptes_client\panoptes.py", line 362, in put
retry=retry,
File "C:\Users\User1\AppData\Local\Programs\Python\Python37-32\lib\site-packages\panoptes_client\panoptes.py", line 281, in json_request
json_response['errors']
panoptes_client.panoptes.PanoptesAPIException: JsonApiController::PreconditionFailed

This workaround does work: Update This ran without errors but did not add a field "Volume" to the subject metadata. It seems I can no longer update subject metadata on existing subjects???

Panoptes.connect(username=os.environ['User_name'], password=os.environ['Password'])
subjects = Subject.where(subject_set_id=67143)
for subject in subjects:
    print(subject.id)
    subj = Subject.find(subject.id)
    subj.metadata['Volume'] = '5'    
    subj.save()
@PmasonFF
Copy link
Author

PmasonFF commented Jul 5, 2019

Even the original code now seems to be working!

Close the issue.

@PmasonFF PmasonFF closed this as completed Jul 5, 2019
@adammcmaster
Copy link
Contributor

I'm reopening this as I just ran into the same issue. It looks like sometimes (but not always) the etag returned by .where() isn't valid when actually trying to save the subject.

Calling .reload() before making any modifications is an easy way to work around it.

@adammcmaster adammcmaster reopened this Nov 6, 2020
@adammcmaster
Copy link
Contributor

To be clear, this isn't an API bug. The client should silently reload the subject as needed.

@camallen
Copy link
Contributor

camallen commented Nov 6, 2020

I'm reopening this as I just ran into the same issue. It looks like sometimes (but not always) the etag returned by .where() isn't valid when actually trying to save the subject.

Calling .reload() before making any modifications is an easy way to work around it.

OOI - the created 201 and any update 200 response should include the updated etag for the resource. I assume calling reload() won't hit the API again but rather just refresh this internal etag state, right?

@adammcmaster
Copy link
Contributor

reload() does hit the API again: https://github.com/zooniverse/panoptes-python-client/blob/master/panoptes_client/panoptes.py#L834 It's the same as calling .find(id) again.

I'm not sure why the etag is wrong sometimes when it's usually fine most of the time.

@camallen
Copy link
Contributor

camallen commented Nov 6, 2020

i wonder if the resource is being updated in the API and thus changing the etag value? I can't think what would be updating the subject behind the scenes API side.... but something might touch that resource.

It would be a waste to add a reload on each save event as it's a double round trip to the API and most likely not needed 90% of the time so would slow client calls down and possibly double the API load for these actions.

Could the client rescue this specific error and retry with a reload() once say, then raise if still errors? This would make the client more robust and cut down on extra API calls / round trips.

@adammcmaster
Copy link
Contributor

Could the client rescue this specific error and retry with a reload() once say, then raise if still errors? This would make the client more robust and cut down on extra API calls / round trips.

I wouldn't want that to happen automatically in the client because it could be dangerous to overwrite whatever changes there could have been, but that's what I'll do to work around the issue here. I'm not really sure what the solution is -- maybe an explicit option on .save() to allow the client to overwrite changes if you know it's safe?

@adammcmaster
Copy link
Contributor

adammcmaster commented Nov 6, 2020

For reference here's how I'm working around it:

                    for retry in (True, False):
                        try:
                            ... make changes to subject ...

                            subject.save()
                        except PanoptesAPIException:
                            if retry:
                                # Reload the subject to refresh the etag
                                # This works around what seems to be a bug in the Panoptes client
                                # https://github.com/zooniverse/panoptes-python-client/issues/220
                                subject.reload()
                            else:
                                raise

@camallen
Copy link
Contributor

camallen commented Nov 6, 2020

Ideally we find the source of this bug eventually but it looks like your workaround will help folks encountering this issue.

@adammcmaster
Copy link
Contributor

One problem with this approach is that the client DOES retry the failing requests with a backoff interval, but they always fail until you call reload(), so it takes the best part of a minute to save each subject this way. I haven't found a good way around that yet unfortunately.

@adammcmaster
Copy link
Contributor

adammcmaster commented Nov 6, 2020

In fact with the backoff it would be fewer requests to just call reload first every time.

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

No branches or pull requests

3 participants