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

Deprecation of a few methods #1025

Merged
merged 10 commits into from
Oct 12, 2022
Merged

Deprecation of a few methods #1025

merged 10 commits into from
Oct 12, 2022

Conversation

Raalsky
Copy link
Contributor

@Raalsky Raalsky commented Oct 3, 2022

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 3, 2022

Test Results

    2 files   -      14      2 suites   - 14   1m 34s ⏱️ - 3h 25m 23s
480 tests  -    209  480 ✔️  -    206  0 💤  -   3  0 ±0 
960 runs   - 1 160  960 ✔️  - 1 136  0 💤  - 24  0 ±0 

Results for commit ff0b26f. ± Comparison against base commit 6782d2f.

This pull request removes 689 and adds 480 tests. Note that renamed tests count towards both.
integrations.test_huggingface.TestHuggingFace ‑ test_every_train_should_create_new_run
integrations.test_huggingface.TestHuggingFace ‑ test_hyperparameter_optimization
integrations.test_huggingface.TestHuggingFace ‑ test_initialization_with_run_provided
integrations.test_huggingface.TestHuggingFace ‑ test_integration_version_is_logged
integrations.test_huggingface.TestHuggingFace ‑ test_log_parameters_disabled
integrations.test_huggingface.TestHuggingFace ‑ test_log_parameters_with_base_namespace
integrations.test_huggingface.TestHuggingFace ‑ test_log_with_custom_base_namespace
integrations.test_huggingface.TestHuggingFace ‑ test_model_checkpoints_best[checkpoint_settings0]
integrations.test_huggingface.TestHuggingFace ‑ test_model_checkpoints_best[checkpoint_settings1]
integrations.test_huggingface.TestHuggingFace ‑ test_model_checkpoints_best[checkpoint_settings2]
…
tests.neptune.alpha_integration.test_alpha_integration_backend.TestAlphaIntegrationNeptuneBackend ‑ test_send_channels_image_values
tests.neptune.alpha_integration.test_alpha_integration_backend.TestAlphaIntegrationNeptuneBackend ‑ test_send_channels_numeric_values
tests.neptune.alpha_integration.test_alpha_integration_backend.TestAlphaIntegrationNeptuneBackend ‑ test_send_channels_text_values
tests.neptune.internal.backends.test_hosted_neptune_backend.TestHostedNeptuneBackend ‑ test_cannot_resolve_host
tests.neptune.internal.backends.test_hosted_neptune_backend.TestHostedNeptuneBackend ‑ test_deprecated_token
tests.neptune.internal.backends.test_hosted_neptune_backend.TestHostedNeptuneBackend ‑ test_max_compatible_version_fail
tests.neptune.internal.backends.test_hosted_neptune_backend.TestHostedNeptuneBackend ‑ test_max_compatible_version_ok
tests.neptune.internal.backends.test_hosted_neptune_backend.TestHostedNeptuneBackend ‑ test_min_compatible_version_fail
tests.neptune.internal.backends.test_hosted_neptune_backend.TestHostedNeptuneBackend ‑ test_min_compatible_version_ok
tests.neptune.internal.backends.test_hosted_neptune_backend.TestHostedNeptuneBackend ‑ test_should_accept_given_api_token
…

♻️ This comment has been updated with latest results.

@@ -159,34 +159,34 @@ def init_run(
>>> import neptune.new as neptune

>>> # minimal invoke
... run = neptune.init()
... run = neptune.init_run()
Copy link
Collaborator

Choose a reason for hiding this comment

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

NB: These examples are reworked by PR #1015 and the change from init to init_run is reflected there already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Let me know once it will be merged so I'll be able to easily get rid of it during rebasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@normandy7 normandy7 left a comment

Choose a reason for hiding this comment

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

Looks good to me, just the one comment about my conflicting PR.


def test_passing_deprecated_parameter_as_none(self):
# pylint: disable=unexpected-keyword-arg
# pylint: disable=missing-kwoa
self.assertIsNone(fun(deprecated_param=None))
Copy link
Contributor

@shnela shnela Oct 6, 2022

Choose a reason for hiding this comment

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

Why did we remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, probably missed that and it should be replaced with assert value is None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Raalsky Raalsky changed the title get_last_run, init, get_project and get_run_url marked as deprecated Deprecation of a few methods Oct 7, 2022
@Raalsky Raalsky force-pushed the rj/deprecated-methods branch 2 times, most recently from 35560bc to 5f506da Compare October 7, 2022 10:05
warnings.simplefilter("always", category=NeptuneDeprecationWarning)


warned_once = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it set instead of dict

# test collision
with self.assertRaises(NeptuneParametersCollision):
fun(new_param=None, deprecated_param=None)
def test_deprecated_func_with_alternative(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add test for warning once ant we're done ;)

@Raalsky Raalsky merged commit f35e4a9 into master Oct 12, 2022
@Raalsky Raalsky deleted the rj/deprecated-methods branch October 12, 2022 11:06
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

Successfully merging this pull request may close these issues.

3 participants