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

Sync only offline runs #894

Merged
merged 13 commits into from
May 11, 2022
Merged

Sync only offline runs #894

merged 13 commits into from
May 11, 2022

Conversation

Blaizzy
Copy link
Contributor

@Blaizzy Blaizzy commented May 10, 2022

No description provided.

Copy link
Contributor

@pkasprzyk pkasprzyk left a comment

Choose a reason for hiding this comment

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

Github doesn't allow me to comment on non-updated lines, so I'll write it here:

I think you can replace

        offline_dirs = get_offline_dirs(base_path)
        self.sync_offline_runs(base_path, project_name, offline_dirs)

in sync_all_containers (ref. https://github.com/neptune-ai/neptune-client/pull/894/files#diff-4d1af3d92b849386a88c5ace8eef8d8f3c60c30caaf857de8a174692ce5e1441R257-R258) with a call to sync_all_offline_containers to avoid code duplication.

Apart from that lgtm.

@Blaizzy
Copy link
Contributor Author

Blaizzy commented May 10, 2022

Thank you @pkasprzyk!

It is done ✅

pkasprzyk
pkasprzyk previously approved these changes May 10, 2022
@pkasprzyk
Copy link
Contributor

@Blaizzy one more thing: could you add changelog entry?

@Blaizzy
Copy link
Contributor Author

Blaizzy commented May 10, 2022

Sure I'll do it 👍🏽

@Blaizzy
Copy link
Contributor Author

Blaizzy commented May 10, 2022

@pkasprzyk I've updated the changelog for the next release.
Please let me know if there are any other improvements.

1 similar comment
@Blaizzy
Copy link
Contributor Author

Blaizzy commented May 10, 2022

@pkasprzyk I've updated the changelog for the next release.
Please let me know if there are any other improvements.

CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
## neptune-client 0.16.2
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing (I haven't mentioned that before, sorry):
When you create a new version entry in CHANGELOG, please mark it with [UNRELEASED], so everyone can tell at a glance that they can add new entries to it. Admittedly we don't ALWAYS do that, but at least try ;). Example: https://github.com/neptune-ai/neptune-client/pull/825/files

@Blaizzy
Copy link
Contributor Author

Blaizzy commented May 11, 2022

Thanks!

I have made the changes ✅

@pkasprzyk pkasprzyk merged commit 806d76d into master May 11, 2022
@pkasprzyk pkasprzyk deleted the pc/sync-only-offline branch May 11, 2022 11:39
normandy7 pushed a commit that referenced this pull request May 23, 2022
* add sync all offline containers arg and logic

* add method to sync all offline containers

* text formatting

* add offline-only arg docstring

* remove default false

* adding default value back

* remove email policy

* remove metavar

* add exception for use of object_names and offline

* black format

* refactor sync_all_containers

* Update changelog for upcoming release

* add unreleased tag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants