Let users never see Entry objects #490

martindurant · 2020-04-29T17:19:57Z

This is "unification light" - still have Entry objects, but the user doesn't see them. The catalog just gives you the default source directly, but you can override them with call or clone (maybe should be get??) to get a new version of the source. Note that .describe() will give you the original catalog definition (with user parameters), not your overridden version, but repr of the source now also gives you that YAML view with the current set of arguments.

cc @danielballan @tacaswell

martindurant · 2020-04-29T17:20:24Z

(I'm sure a load of tests would need updating, but would like comments sooner rather than later)

tacaswell · 2020-04-29T18:02:58Z

intake/source/base.py

+    def describe(self):
+        return self.entry.describe()
+
+    @property


This seems like an application layer thing that should not be baked into the core objects. If this is something that was there and needs to stay for back compatibility can we deprecate it?

Right, you could construct a GUI for an entry previously (which allowed you to select the user parameters and then plot with point&click); so if the user is no longer exposed to the entry object, this is the only way to do it. Of course, we could question the usefulness of that particular GUI view, but otherwise the source no longer has any connection to the GUIs at all.

It makes a lot more sense to me to pass the DateSrouce to the gui (make_the_clicky_gui(ds)) rather than have it be a method so that it is possible to support exposing several gui generation tactics to the user (ex "all the details" vs "executive summary").

I too would like to revisit this, but I think should not lump that into this PR.

The GUI code can probably do with a refactor following improvements in https://github.com/intake/filesystem_spec/blob/master/fsspec/gui.py#L14

intake/source/base.py

intake/catalog/entry.py

tacaswell · 2020-04-29T18:35:21Z

Sorry for crashing into your code base with possibly more opinions than understanding.

martindurant · 2020-04-29T18:52:31Z

Sorry for crashing into your code base with possibly more opinions than understanding.

It is appreciated!

danielballan · 2020-04-29T23:25:32Z

Thanks for leading the way on this, @martindurant! I am still under a rock but should emerge next week to look at this.

martindurant · 2020-05-11T16:55:20Z

Good to go, reviews appreciated!

(docs still need updating in some places; may have knock-on errors in dependent packages which need sorting before blanket releases)

danielballan · 2020-05-11T18:40:04Z

I'd like to beg another week or so to look at this. Racing to prep for a big tutorial a week from tomorrow (unveiling databroker-refactored-on-top-of-intake among other things).

martindurant · 2020-05-12T17:47:59Z

@danielballan , I can wait :)

intake/utils.py

danielballan

I am convinced that this approach is better than the one I had in mind. Retaining Entry as an internal entity will be useful for a least two reasons I can think of:

making new instances with different parameters cleanly
giving us a bundle of parameters that we can pickle, whereas DataSource instances themselves may carry un-pickleable resources like file handles or sockets

Also, this is clearly as an easier refactor than attempting to excise Entry from the codebase. :-)

I have a couple comments for your consideration. I would also like to test databroker against this branch before it is merged. Will post results today or tomorrow.

intake/catalog/entry.py

intake/source/base.py

danielballan · 2020-05-21T14:16:40Z

intake/source/base.py

+    def describe(self):
+        return self.entry.describe()
+
+    @property


I too would like to revisit this, but I think should not lump that into this PR.

intake/source/base.py

intake/utils.py

danielballan · 2020-05-21T21:28:29Z

This requires some small changes to databroker but I now have a branch bluesky/databroker#569 that passes against this intake PR branch, intake master, and intake 0.5.5. I also took it for a spin interactively, and it behaves as expected.

Remaining concerns:

There is a back-compatibility issue with .get(). See this example using data-us-states. With intake 0.5.5:

In [3]: intake.cat.states.get()                                                                                                                                                               
Out[3]: <intake.source.csv.CSVSource at 0x7ff9771b50d0>

With this PR branch

In [3]:  intake.cat.states.get()                                 
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-5-e1dc9e0715d2> in <module>
----> 1 intake.cat.states.get()

AttributeError: 'CSVSource' object has no attribute 'get'

Settling on a name for configure (I am fine with copy_and_configure, or other suggestions listed in line above.)
Removing sorted or making it conditional on Python version (again see comment above)

Thanks again @martindurant for taking this on.

martindurant · 2020-05-22T15:47:02Z

Last call for comments

martindurant · 2020-05-22T15:47:26Z

(updates to docs will follow merger and release)

danielballan · 2020-05-22T19:13:31Z

I'm seeing a regression from the last commit but I don't understand it yet. Please hold merging while I investigate.

danielballan · 2020-05-29T13:53:28Z

Looks like it was just a confusion on my end, I must have had different branches installed than I thought. Full steam ahead!

danielballan · 2020-05-29T13:58:20Z

Rebased to resolve conflicts and force-pushed

martindurant · 2020-05-29T14:00:28Z

Rebased to resolve conflicts and force-pushed

Did the same, but you were faster!

To https://github.com/martindurant/intake
 ! [rejected]        unify -> unify (fetch first)
error: failed to push some refs to 'https://github.com/martindurant/intake'

danielballan · 2020-05-29T14:07:28Z

Welp, looks like I introduced a syntax error in rebase....

martindurant · 2020-05-29T14:10:35Z

fixed...

danielballan · 2020-05-29T14:21:16Z

Test failure is on one build, GUI related.

martindurant · 2020-05-29T14:47:24Z

You took the button press away from me! :)

tacaswell reviewed Apr 29, 2020

View reviewed changes

intake/source/base.py Outdated Show resolved Hide resolved

tacaswell reviewed Apr 29, 2020

View reviewed changes

intake/source/base.py Outdated Show resolved Hide resolved

martindurant commented Apr 29, 2020

View reviewed changes

intake/source/base.py Outdated Show resolved Hide resolved

tacaswell reviewed Apr 29, 2020

View reviewed changes

intake/catalog/entry.py Outdated Show resolved Hide resolved

martindurant mentioned this pull request May 6, 2020

Zarr catalogs #491

Merged

martindurant force-pushed the unify branch from d5e6a80 to 47bc401 Compare May 11, 2020 16:48

martindurant changed the title ~~WIP: Let users never see Entry objects~~ Let users never see Entry objects May 11, 2020

tacaswell reviewed May 21, 2020

View reviewed changes

intake/utils.py Outdated Show resolved Hide resolved

danielballan reviewed May 21, 2020

View reviewed changes

danielballan mentioned this pull request May 21, 2020

Adjust for Entry unification bluesky/databroker#569

Merged

Martin Durant added 7 commits May 29, 2020 09:54

Let users never see Entry objects

83cbfcf

Remove magic attr, add to entry attrs to source

6178550

fix call

7426939

Remove weakref stuff

ddcbda2

fixed many tests

1827c2a

fix all but

afe81ff

fix plot test

c4e9087

Martin Durant added 2 commits May 29, 2020 09:55

revert and explicit persist clear

e6aafd4

response and fix

99733db

danielballan force-pushed the unify branch from 5d88def to 99733db Compare May 29, 2020 13:58

fix

3135364

follow-on

94f72f7

danielballan merged commit 48360c9 into intake:master May 29, 2020

martindurant deleted the unify branch May 29, 2020 14:47

martindurant mentioned this pull request Jun 4, 2020

Allow parameters in urlpath with zarr entries #505

Open

This was referenced Jun 9, 2020

Add df property setter intake/intake-esm#247

Merged

Sync with intake's Entry unification intake/intake-esm#249

Merged

ian-r-rose mentioned this pull request Jul 7, 2020

Performance in loading nested catalogs #512

Closed

danielballan mentioned this pull request Nov 5, 2020

Expect entry or source to implement 'container' but not '_container'. #548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let users never see Entry objects #490

Let users never see Entry objects #490

martindurant commented Apr 29, 2020

martindurant commented Apr 29, 2020

tacaswell Apr 29, 2020

martindurant Apr 29, 2020

tacaswell Apr 29, 2020

danielballan May 21, 2020

martindurant May 21, 2020

tacaswell commented Apr 29, 2020

martindurant commented Apr 29, 2020

danielballan commented Apr 29, 2020

martindurant commented May 11, 2020

danielballan commented May 11, 2020

martindurant commented May 12, 2020

danielballan left a comment

danielballan May 21, 2020

danielballan commented May 21, 2020 •

edited by martindurant

martindurant commented May 22, 2020

martindurant commented May 22, 2020

danielballan commented May 22, 2020

danielballan commented May 29, 2020

danielballan commented May 29, 2020

martindurant commented May 29, 2020

danielballan commented May 29, 2020

martindurant commented May 29, 2020

danielballan commented May 29, 2020

martindurant commented May 29, 2020

Let users never see Entry objects #490

Let users never see Entry objects #490

Conversation

martindurant commented Apr 29, 2020

martindurant commented Apr 29, 2020

tacaswell Apr 29, 2020

Choose a reason for hiding this comment

martindurant Apr 29, 2020

Choose a reason for hiding this comment

tacaswell Apr 29, 2020

Choose a reason for hiding this comment

danielballan May 21, 2020

Choose a reason for hiding this comment

martindurant May 21, 2020

Choose a reason for hiding this comment

tacaswell commented Apr 29, 2020

martindurant commented Apr 29, 2020

danielballan commented Apr 29, 2020

martindurant commented May 11, 2020

danielballan commented May 11, 2020

martindurant commented May 12, 2020

danielballan left a comment

Choose a reason for hiding this comment

danielballan May 21, 2020

Choose a reason for hiding this comment

danielballan commented May 21, 2020 • edited by martindurant

martindurant commented May 22, 2020

martindurant commented May 22, 2020

danielballan commented May 22, 2020

danielballan commented May 29, 2020

danielballan commented May 29, 2020

martindurant commented May 29, 2020

danielballan commented May 29, 2020

martindurant commented May 29, 2020

danielballan commented May 29, 2020

martindurant commented May 29, 2020

danielballan commented May 21, 2020 •

edited by martindurant