Skip to content

Support DVC legacy user_id file path #31

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Support DVC legacy user_id file path #31

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6927957
Move to new path
omesser Jul 4, 2022
b3c8df8
Another pass
omesser Jul 4, 2022
7fed0c0
addressing CR
omesser Jul 4, 2022
3c593d7
Add _try_write_legacy_user_id logic, move to pathlib
omesser Jul 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ venv/
ENV/
env.bak/
venv.bak/
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

😄 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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


# Spyder project settings
.spyderproject
Expand Down
21 changes: 19 additions & 2 deletions src/iterative_telemetry/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ def _find_or_create_user_id():
IDs are generated randomly with UUID.
"""

config_dir = user_config_dir("telemetry", "iterative")
config_dir = user_config_dir(
os.path.join("iterative", "telemetry"), "iterative"
)
Copy link
Contributor

@efiop efiop Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

  • 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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Author

@omesser omesser Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@casperdcl casperdcl Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

@omesser omesser Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@omesser omesser Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

fname = os.path.join(config_dir, "user_id")
lockfile = os.path.join(config_dir, "user_id.lock")

Expand All @@ -213,7 +215,9 @@ def _find_or_create_user_id():
user_id = json.load(fobj)["user_id"]

except (FileNotFoundError, ValueError, KeyError):
user_id = str(uuid.uuid4())

# Backwards compatibility with DVC legacy telemetry location.
user_id = _try_read_legacy_user_id() or str(uuid.uuid4())

with open(fname, "w", encoding="utf8") as fobj:
json.dump({"user_id": user_id}, fobj)
Expand All @@ -223,3 +227,16 @@ def _find_or_create_user_id():
except Timeout:
logger.debug("Failed to acquire %s", lockfile)
return None


def _try_read_legacy_user_id():
fname = os.path.join(user_config_dir("dvc", "iterative"), "user_id")

try:
with open(fname, encoding="utf8") as fobj:
return json.load(fobj)["user_id"]

except (FileNotFoundError, ValueError, KeyError):
pass

return None