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

Db export, again #88

Merged
merged 17 commits into from
Nov 15, 2016
Merged

Db export, again #88

merged 17 commits into from
Nov 15, 2016

Conversation

danielballan
Copy link
Member

This is a rebase of #76 with some extra commits to add a test and make it pass.

@codecov-io
Copy link

codecov-io commented Sep 15, 2016

Current coverage is 27.93% (diff: 2.63%)

Merging #88 into master will decrease coverage by 53.46%

@@             master        #88   diff @@
==========================================
  Files             6          6          
  Lines           661        691    +30   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            538        193   -345   
- Misses          123        498   +375   
  Partials          0          0          

Powered by Codecov. Last update d8022be...489fec5

@danielballan danielballan mentioned this pull request Sep 15, 2016
@danielballan
Copy link
Member Author

This may be ready. It could use a careful read from more than one reviewer. As you can see, the outline for copying external files is here, but not finished. (The docs include warning about that.) I'd like to roll this out as is and handle the external files piece in a second pass.

res['resource_path'],
res['resource_kwargs'],
root=new_root)
# FIXME: revisit root when dealing
Copy link
Contributor

@licode licode Sep 15, 2016

Choose a reason for hiding this comment

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

The new_root is the root place where the external data file is going to transferred? So basically we can transfer data between different servers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

@danielballan
Copy link
Member Author

@licode Yes, that is the plan. As written so far, this only copies database documents between servers. A future iteration will make use of the new_root argument to copy files as well.

@licode
Copy link
Contributor

licode commented Sep 15, 2016

Got it. thanks.
Can we detect the root path automatically? So we do not need to manually change root, or keep this as an optional argument.

@danielballan
Copy link
Member Author

How would that work?

Get Outlook for iOShttps://aka.ms/o0ukef

On Thu, Sep 15, 2016 at 3:52 PM -0400, "Li Li" <notifications@github.commailto:notifications@github.com> wrote:

Got it. thanks.
Can we detect the root path automatically? So we do not need to manually change root, or keep this as an optional argument.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com/NSLS-II/dataportal/pull/88#issuecomment-247433859, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACLIrh5G6yfFuReEvkfFjzXQYK6mgaOpks5qqaHbgaJpZM4J-G9z.

@licode
Copy link
Contributor

licode commented Sep 15, 2016

I think I oversimplify the issue. Please ignore that.

@licode
Copy link
Contributor

licode commented Sep 15, 2016

👍 This looks good to me

db.fs.insert_resource(res['spec'],
res['resource_path'],
res['resource_kwargs'],
root=new_root)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should pass through res['root'] until this is actually implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note to anyone following along: this is now actually implemented.

headers = [headers]
for header in headers:
# insert mds
db.mds.insert_run_start(**_clean_doct(header['start']))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

and by 'this' I mean _clean_doct

Copy link
Member Author

Choose a reason for hiding this comment

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

Because a doct has a _name attribute hanging off of it that gets unpacked in insert_run_start.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am pretty sure we should get rid of '_name' -- an artifact of mongoengine documents -- but that's a question for another day.

@@ -799,3 +855,10 @@ def _munge_time(t, timezone):
"""
t = datetime.fromtimestamp(t)
return timezone.localize(t).replace(microsecond=0).isoformat()


def _clean_doct(d):
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a function in doct for doing this splitting.

@danielballan
Copy link
Member Author

This is close, but it requires a FileStore that implements resource_given_eid. @tacaswell only implemented this method on FileStoreMoving, which means that users would need to set up FileStoreMoving on the production databroker before exporting. It seem this is actually encouraging the opposite of careful behavior, but I think @tacaswell has some opinions about these methods, so I'll leave the rest to him.

@danielballan
Copy link
Member Author

If NSLS-II/filestore#121 is merged, this PR should just work. But I don't want to run roughshod over any concerns @tacaswell may have here, so please do not merge unless you are @tacaswell. :- )

@@ -190,7 +191,7 @@ def get_events(mds, fs, headers, fields=None, stream_name=ALL, fill=False,
# Look in the descriptor, then start, then stop.
config_data_fields = set(filter(comp_re.match, config_data)) - selected_fields
for field in config_data_fields:
selected_fields.append(field)
selected_fields.add(field)
Copy link
Member Author

Choose a reason for hiding this comment

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

This part is an unrelated bug fix.

@danielballan
Copy link
Member Author

Note to whoever has the patience to finish this someday: FileStore.datum_gen_given_resource calls core.get_datumkw_by_resuid_gen which returns the kwargs from the datums but successfully obscures the datum_ids. FileStore is designed to make it hard to get this stuff, and that aspect of the design is proving to be a wild success.

res['resource_path'],
res['resource_kwargs'],
root=new_root)
datums = self.fs.datum_gen_given_resource(uid)
Copy link
Member Author

Choose a reason for hiding this comment

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

NSLS-II/filestore#122 will make this method,datum_gen_given_resource, return the full datum document (including the ID which we need) instead of just the datum kwargs.

@danielballan
Copy link
Member Author

Test passes locally and should pass here once NSLS-II/filestore#122 and bluesky/bluesky#612 are in. Phew.

@tacaswell
Copy link
Contributor

Not wanting to feel left out I added a commit to this PR too!

@danielballan
Copy link
Member Author

Test failure is unrelated to this PR but probably real. I'll open a separate issue and expand on that.

Seeing @tacaswell's marked approval above, and knowing that three of us have code in here, I guess I'll be the one to pull the trigger....

@danielballan danielballan merged commit b2bb5fa into bluesky:master Nov 15, 2016
@danielballan danielballan deleted the db_export branch November 15, 2016 04:00
"""
Export a list of headers.

.. warning::
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this warning still apply?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not

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.

6 participants