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

Change checkpoint to be checkpoint(name, tags...; data...) #16

Open
oxinabox opened this issue May 7, 2021 · 6 comments
Open

Change checkpoint to be checkpoint(name, tags...; data...) #16

oxinabox opened this issue May 7, 2021 · 6 comments

Comments

@oxinabox
Copy link
Member

oxinabox commented May 7, 2021

As of #15 (cc @mzgubic)
we are allowed duplicate tags, and because of this with_tag is used as with_tag(:tag1=1, :tag2=2) do
Since we need to actually have a function that can take those duplicate tags they can't be in the keyword positon.

Conversely, the data keys must be unique, so having them in the varargs position is suboptimal.

Also as of #15 it will be very rare to pass tags directly to a checkpoint.
So calls will be checkpoint("FooBar"; foo=1, bar=[1,2,3]) which will create JLSO files containing both foo and bar.
In a location determined by the context tags

@mzgubic
Copy link
Contributor

mzgubic commented May 7, 2021

Sounds like the right thing to do. But also sounds like it it pretty low priority?

@oxinabox
Copy link
Member Author

oxinabox commented May 7, 2021

Correct, it is a breaking change.
We should not do it til we have a better reason to make a breaking change.

@rofinn why the 👎 ?

@oxinabox
Copy link
Member Author

A cool advantage of this is checkpoint("FooBar", :foo=>foo) could be written as checkpoint("FooBar"; foo) taking advantage of the julia 1.6 (or was it 1.5) feature that automatically names kwargs.

@rofinn
Copy link
Member

rofinn commented May 12, 2021

  1. I'm not a fan of the checkpoint("FooBar"; foo) convention that's been introduced as it seems easy to introduce a single character error :trollface:
  2. Seems like a kwargs nightmare if we want to start adding kwarg settings to checkpoint.
  3. Seems like more unnecessary syntax, similar to using macros just to extract the variable name (extracting the filename and number is the primary argument I can see)

@oxinabox
Copy link
Member Author

oxinabox commented May 13, 2021

  1. I'm not a fan of the checkpoint("FooBar"; foo) convention that's been introduced as it seems easy to introduce a single character error :trollface:

I might be missing the troll.
Doesn't it decreae the chance of single character error, since now you a.) type the name once rather than :foo=>foo, and b.) it is the same name is the rest of your surounding code (which is assumably right) so it will tab-complete

I found the (;foo) convention weird at first but it has grown on me.

  1. Seems like a kwargs nightmare if we want to start adding kwarg settings to checkpoint.

I am going to make the bold claim that we don't ever want to add kwargs settings to checkpoint. For two reasons

  1. The majority of settings we might want to add should be configurable by the end user when enabling the checkpoints, so should be argumebts to config.
  2. any kind of args we want to add seems likely to be mode-switching, in which case want them to be positional arguments so we can use dispatch on them.
  1. Seems like more unnecessary syntax,

It's less syntax :foo=>3 vs foo=3.
I guess a debate could be had as to if symbol literals and Pairs are a more "advanced" feature than kwargs, but I think both are surficiently basic that it doesn't matter.

@rofinn
Copy link
Member

rofinn commented Jun 9, 2021

Doesn't it decreae the chance of single character error, since now you a.) type the name once rather than :foo=>foo, and b.) it is the same name is the rest of your surounding code (which is assumably right) so it will tab-complete

Yes, but I think identifying a typo in foo is a lot easier than realizing that ("FooBar", foo) needs to be ("FooBar"; foo)

I am going to make the bold claim that we don't ever want to add kwargs settings to checkpoint.

I'm reluctant to say that we "don't ever want to" do something.

The majority of settings we might want to add should be configurable by the end user when enabling the checkpoints, so should be argumebts to config.

That's generally true apart from things that might be data related settings. For example, a Handler may be able to store the data more efficiently given extra information about what's being stored (e.g., explicitly enable/disable data deduplication based on know of the access patterns at the call-site). Ideally, most of that info would be determined by inspection of the data itself, but sometimes context or overrides will be required.

any kind of args we want to add seems likely to be mode-switching, in which case want them to be positional arguments so we can use dispatch on them.

I'm not convinced on this. I see the argument for handlers to support dispatching on the type of the data itself, but the patterns I've seen in the Julia ecosystem for mode switching arguments should be kwargs (e.g., dims=1, corrected=true, digits=2, base=10).

It's less syntax

Sorry, the point I'm trying to make is that we already have a convention for constructing associative types with :foo=>3 when constructing a dictionary. My dislike of the kwarg pattern used in DataFrames and a few other packages is that it introduces yet another way in which we construct associative types specifically if the keys happen to be symbols.

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

No branches or pull requests

3 participants