Skip to content

Conversation

nagem
Copy link
Contributor

@nagem nagem commented Jun 28, 2016

Addresses #159, #160 and a few other requests made by team members.

Changes:

  • Engine uploads now use the Placer class style
  • Uploads can be made to projects, sessions, acquisitions or analyses*
  • Uploads can be files only
  • Uploads can be metadata only

*Work towards supporting uploads to multiple levels of the hierarchy at once (#160) is being done in a separate branch

@nagem
Copy link
Contributor Author

nagem commented Jun 28, 2016

Will update the endpoint documentation on next commit

session_obj = None
if update_timestamp:
update['$min'] = dict(timestamp=container['timestamp'])
session['timezone'] = dict(timezone=container.get('timezone'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This min/max setting of the session and project timestamps was in the existing code. Unless someone has a reason to keep it, I'd like to remove it. @rentzso @gsfr

Copy link
Member

Choose a reason for hiding this comment

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

The UI renders the session timestamp, so I suggest we keep setting it. It is currently defined to be equal to the oldest acquisition timestamp.

The project timestamp is not used by the UI, as far as I know, and also what we are doing doesn't make a ton of sense, so we could get rid of that part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is currently defined to be equal to the oldest acquisition timestamp.

Thanks, that's helpful information. A user can set the timestamp, would we want to overwrite a user's input? Unfortunately we wouldn't be able to differentiate between user input and machine input at this time (as we were discussing), so the question is more "is overwriting a session's timestamp with metadata ever okay?"

Copy link
Member

Choose a reason for hiding this comment

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

After offline conversation, we conclude that the engine placer will only do as it is told and not have side effects, such as updating session timestamps without being explicitly told to do so. Reaper uploads are a different story, though. Side effect will remain there.

@kofalt
Copy link
Contributor

kofalt commented Jun 28, 2016

Looks good so far. Ping me when changes are done

@kofalt
Copy link
Contributor

kofalt commented Jun 28, 2016

  File "webapp2.py", line 570, in dispatch
    return method(*args, **kwargs)
  File "./api/upload.py", line 236, in engine
    return process_upload(self.request, Strategy.engine, container_type=level, id=cid, origin=self.origin)
  File "./api/upload.py", line 141, in process_upload
    return placer.finalize()
  File "./api/placer.py", line 219, in finalize
    self.obj = hierarchy.update_container_hierarchy(self.metadata, bid, self.container_type)
  File "./api/dao/hierarchy.py", line 333, in update_container_hierarchy
    _update_hierarchy(c_obj, container_type, metadata, update_timestamp)
  File "./api/dao/hierarchy.py", line 358, in _update_hierarchy
    session_obj = _update_container({'_id': container['session']}, update, session, 'sessions')
  File "./api/dao/hierarchy.py", line 341, in _update_container
    return_document=pymongo.collection.ReturnDocument.AFTER
  File "pymongo/collection.py", line 2155, in find_one_and_update
    sort, upsert, return_document, **kwargs)
  File "pymongo/collection.py", line 1937, in __find_and_modify
    allowable_errors=[_NO_OBJ_ERROR])
  File "pymongo/collection.py", line 205, in _command
    read_concern=read_concern)
  File "pymongo/pool.py", line 211, in command
    read_concern)
  File "pymongo/network.py", line 100, in command
    helpers._check_command_response(response_doc, msg, allowable_errors)
  File "pymongo/helpers.py", line 196, in _check_command_response
    raise OperationFailure(msg % errmsg, code, response)

OperationFailure: command SON([('findAndModify', u'sessions'), ('query', {'_id': ObjectId('5772dc4c669becae4325338f')}), ('new', True), ('update', {'$set': {u'subject.sex': u'male', u'subject.firstname_hash': None, u'timestamp': u'1970-01-01T00:00:00+00:00', u'subject.lastname': u'', 'modified': datetime.datetime(2016, 6, 28, 20, 23, 21, 805863), u'subject.firstname': u'', u'subject.lastname_hash': None, u'subject.age': 0.0, 'timezone.timezone': None, u'operator': None}, '$min': {'timestamp': datetime.datetime(1970, 1, 1, 0, 0)}}), ('upsert', False)]) on namespace scitran.$cmd failed: exception: Cannot update 'timestamp' and 'timestamp' at the same time

Cannot update 'timestamp' and 'timestamp' at the same time

Mongo does not approve 😣

@nagem
Copy link
Contributor Author

nagem commented Jul 5, 2016

Non-explicit timestamp setting removed. Will rerun a few tests using the engine and then this is ready for merge.

The mongo error above was caused by setting the timestamp in a $min update and a $set update at the same time, which has been removed.

@kofalt
Copy link
Contributor

kofalt commented Jul 6, 2016

I'll give this a test today.

@kofalt
Copy link
Contributor

kofalt commented Jul 6, 2016

This update looks to have a bug that removes or overwrites container files on upload. An acquisition that had a dicom zip lost that file:

    "files": [
        {
            "name": "1_1_nifti.nii.gz", 
            "origin": {
                "id": null, 
                "type": "unknown"
            }, 
            "type": "nifti"
        }
    ], 

@nagem
Copy link
Contributor Author

nagem commented Jul 6, 2016

Fixed issue with metadata for files overwriting what's there during update_hierarchy step, which also fixed missing origin. Added a test for that specific situation.

Reruning the engine in a newly bootstrapped environment is also successful.

@kofalt
Copy link
Contributor

kofalt commented Jul 6, 2016

Latest changes work; LGTM

@nagem
Copy link
Contributor Author

nagem commented Jul 6, 2016

Thanks for reviewing, merging now.

@nagem nagem merged commit 130ea9b into master Jul 6, 2016
@nagem nagem deleted the engine-placer branch July 6, 2016 20:47
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.

4 participants