Support DVC legacy user_id file path

[omesser]

Closes #13

• Copy over user_id to new file path (@mike0sv please note) - after some discussion with @efiop changed the approach of only attempting this once, so please note this extra lookup will happen on each run if the happy path is not fulfilled - it's probably ok (another potentially redundant fs read). If bothers you/anyone we need to workout the condition I guess
• Note: No need to copy stuff back to DVC's old path - when this will be released - the goal is that DVC will import and use it for the purposes of determining the telemetry config file along with other tools (MLEM, GTO)
• Added .idea to .gitignore to support non-cool people like me, preferring Pycharm as an IDE
• There's also the "issue(s)" of moving to pathlib (nicer) and some naming beefs I have (fname...) - will address in dedicated PR

[efiop]

Could you explain what was wrong with the previous approach? user_config_dir is following particular convention and it is kinda strange that we use iterative/telemetry as an app name here.

[omesser]

• It's the path from rename user_id file dvc#7707 which I assumed was agreed upon
• Current code user_config_dir("telemetry", "iterative") creates config dir with telemetry (somewhere in its path). appauthor only used on windows system from the docstring, so I'm guessing there will be no reasonable namespace

[efiop]

@omesser So maybe

Suggested change

	config_dir = user_config_dir(
	os.path.join("iterative", "telemetry"), "iterative"
	)
	config_dir = user_config_dir("iterative-telemetry", "iterative")

then? The os.path.join pretty much hacks the convention and might not be desirable. dvc#7707 just talks about it being consistent, I'm sure particular location can be flexible, as long as it is constant and follows conventions. telemetry might be a pretty generic name, so iterative-telemetry helps identify it better. But maybe we could invent a better name for iterative-telemetry (might be too late now though).

Also this change will probably break backward compatibility for MLEM? cc @mike0sv

[omesser]

Also this change will probably break backward compatibility for MLEM? cc @mike0sv

Yes, but that's probably early enough in the game to be ok (?)

about the new path - I don't feel strongly either way (because we can change it again), but having an iterative config top dir makes a lot of sense to me, this is a namespace, and we can nest other config "subdomains" there in the future - like tool specific config, or "project" related configurations. so It's cleaner in that sense than dasherizing

[shcheklein]

It's not MLEM alone, this is VS Code also. I remember we had this discussion in Notion and agreed on something like

e.g. ~/.local/share/iterative/telemetry so we differentiate users rather than tools×users.

https://www.notion.so/iterative/Telemetry-Specification-1a0197b8e34d43a89777a4c9a00ad1fe?d=46ce025aef734858a653d0c485d4e896#ddef310469b44c13a445c686f73014d4

(I remember we also had a discussion with @mattseddon on this already but don't remember the details)

[efiop]

@omesser Great find! Ok, so I guess we should indeed use the same here. Waiting for @mike0sv to confirm then.

[casperdcl]

CML/TPI use:

Suggested change

	config_dir = user_config_dir(
	os.path.join("iterative", "telemetry"), "iterative"
	)
	config_dir = user_config_dir(os.path.join("iterative", "telemetry"), False)

for maximum cross-platform compatibility.

Anything else would break currently released TPI.

Suggest #32 instead

[omesser]

Thanks @casperdcl - gonna add the code writing to the old path as well here, and was planning to move to pathlib in followup PR to minimize changes / style here, but, probably gonna reconsider and shove it in here now :).
Let's see how the format thread go I suppose - I don't have any strong feelings in terms of which PR is used of course, if you want to go with #32 we need to move discussions there

[mike0sv]

not sure what was decided? to leave iterative/telemetry?

[omesser]

That's my understanding, because vs-code is doing iterative/telemetry and that was also what was decided in https://www.notion.so/iterative/Telemetry-Specification-1a0197b8e34d43a89777a4c9a00ad1fe?d=46ce025aef734858a653d0c485d4e896#ddef310469b44c13a445c686f73014d4

[efiop]

LGTM, except for the config discussion above

[casperdcl]

I thoroughly dislike any OS and IDE-specific things appearing in .gitignore.
About 50% of this file is noxious to me :)

The world does not need this cruft in every repo. Better to modify your system-wide git config. Possibly a topic for a follow-up issue/discussion?

[omesser]

😄 Doing this in the repo's git-tracked .gitignore is still the convention in virtually every project I've seen, personally.
Making assumptions about contributors global git config is probably too risky - and projects need to be robust to people configuring their systems/dev-boxes whichever way they like

[mike0sv]

agreed with @omesser - dont see any downsides to have a couple more lines in .gitignore for all popular IDEs. also this repo was generated from our template so might be worth to add it there also https://github.com/iterative/py-template

[casperdcl]

srry this implementation isn't right. The old file was JSON, the new file is not JSON. It's more like a .git/refs file.

Correct implementation:

old = Path(user_config_dir("dvc/user_id", "iterative"))   # backwards-compatibility
new = Path(user_config_dir("iterative/telemetry", False)) # cross-product path
uid = generate_id()
if new.exists():
    uid = new.read_text().strip()
else:  # if uid.lower() != "do-not-track":
    if old.exists():
        uid = json.load(old.open())["user_id"]
    new.write_text(uid)

[omesser]

srry this implementation isn't right. The old file was JSON, the new file is not JSON. It's more like a .git/refs file.

Correct implementation:

old = Path(user_config_dir("dvc/user_id", "iterative"))   # backwards-compatibility
new = Path(user_config_dir("iterative/telemetry", False)) # cross-product path
uid = generate_id()
if new.exists():
    uid = new.read_text().strip()
else:  # if uid.lower() != "do-not-track":
    if old.exists():
        uid = json.load(old.open())["user_id"]
    new.write_text(uid)

VSCode is also implementing json formatted configs today (I thought the write_text() was a mistake / oversight)
What was the format agreement? @shcheklein @casperdcl ? What's the main case for the non-JSON format?

[casperdcl]

Ok if we're already using JSON in two places let's stick with it - I've updated #32 accordingly.

Haven't checked the recent changes here (#31) - hoping the two PRs are effectively the same?

[efiop]

@mike0sv are you using this package in mlem or elsewhere? If so, are these changes to the location okay with you?

EDIT: I see you've commented in #32 already, nevermind

[mike0sv]

Should we close this in favor of #32?

[mike0sv]

or the other way around? :)

[casperdcl]

Had a chat with @omesser - TL;DR we're closing this one. I've ported over comments to the other one, please do review :)