Skip to content

init: throw on dvc/dvclive config mismatch #74

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

Jump to bottom
Merged
merged 3 commits into from
Jul 29, 2021
Merged

init: throw on dvc/dvclive config mismatch #74

merged 3 commits into from
Jul 29, 2021
+58 −16

Conversation

pared
Copy link
Contributor

@pared pared commented May 13, 2021 •
edited
Loading

Fixes #60

TODO
- [x] - allow overriding if proper env variable is present
- [x] - If same dir is provided via env, make sure to update config

EDIT:
we agreed in #74 to throw on configuration mismatch

@codecov-commenter
Copy link

codecov-commenter commented May 13, 2021 •
edited
Loading

Codecov Report

Merging #74 (be8defb) into master (8755731) will increase coverage by 0.27%.
The diff coverage is 95.65%.

❗ Current head be8defb differs from pull request most recent head 8ba8040. Consider uploading reports for the commit 8ba8040 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   87.50%   87.77%   +0.27%     
==========================================
  Files          10       10              
  Lines         256      270      +14     
==========================================
+ Hits          224      237      +13     
- Misses         32       33       +1
Impacted Files Coverage Δ
dvclive/error.py 92.30% <87.50%> (-7.70%) ⬇️
dvclive/__init__.py 100.00% <100.00%> (ø)
dvclive/metrics.py 94.39% <100.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8755731...8ba8040. Read the comment docs.

@pared pared changed the title [WIP] init: override init call with env variables init: override init call with env variables May 17, 2021
@pared pared force-pushed the 60_override branch 4 times, most recently from 9be7b07 to e5819da Compare May 18, 2021 11:18
@pared pared requested a review from jorgeorpinel May 18, 2021 12:51
jorgeorpinel
jorgeorpinel reviewed May 19, 2021
View reviewed changes
jorgeorpinel
jorgeorpinel reviewed May 19, 2021
View reviewed changes
Copy link

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Do you need my approval code-wise though? May be best to ask another core member anyway (additionally) once the behavior is clear at least. I'll continue discussing in #60 for now.

jorgeorpinel
jorgeorpinel reviewed May 19, 2021
View reviewed changes
This was referenced May 19, 2021
Closed
Closed
@pared
Copy link
Contributor Author

pared commented May 20, 2021

Do you need my approval code-wise though?

@jorgeorpinel I was rather thinking about discussing the behavior. Lets continue it under #60

@pared pared changed the title init: override init call with env variables [WIP] init: override init call with env variables May 20, 2021
jorgeorpinel
jorgeorpinel approved these changes May 21, 2021
View reviewed changes
Copy link

@jorgeorpinel jorgeorpinel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Happy to approve this. TBH I didn't test it, but the purported change should indeed address #60 up to what we've discussed so far.

@pared pared changed the title [WIP] init: override init call with env variables init: throw on dvc/dvclive config mismatch Jun 3, 2021
@pared pared changed the title init: throw on dvc/dvclive config mismatch [WIP] init: throw on dvc/dvclive config mismatch Jun 3, 2021
@daavoo
Copy link
Contributor

daavoo commented Jul 14, 2021

@pared Is this still [WIP] ?

@pared
Copy link
Contributor Author

pared commented Jul 20, 2021

@daavoo sorry for the late response, yes, I need to finish this change according to the discussion under #60 I will get to it after later this week.

@efiop efiop assigned pared and daavoo and unassigned pared Jul 27, 2021
@pared pared changed the title [WIP] init: throw on dvc/dvclive config mismatch init: throw on dvc/dvclive config mismatch Jul 28, 2021
@pared pared requested a review from daavoo July 28, 2021 11:36
daavoo
daavoo reviewed Jul 28, 2021
View reviewed changes
@daavoo daavoo merged commit 280800a into master Jul 29, 2021
@daavoo daavoo deleted the 60_override branch July 29, 2021 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgeorpinel jorgeorpinel

@daavoo daavoo

Assignees

@daavoo daavoo

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should the dvc stage add --live <path> arg override init(path)?
4 participants
@pared @codecov-commenter @daavoo @jorgeorpinel