chore: Improve core v2 init API [MD-441] #9560

gt2345 · 2024-06-24T21:15:27Z

Ticket

MD-441

Description

Deprecate DefaultConfig and UnmanagedConfig in core_v2 init API, and use Config instead.

Test Plan

e2e_test pass
Running unmanaged experiment using previous config also works

Checklist

Changes have been manually QA'd
New features have been approved by the corresponding PM
User-facing API changes have the "User-facing API Change" label
Release notes have been added as a separate file under docs/release-notes/
See Release Note for details.
Licenses have been included for new code which was copied and/or modified from any external code

netlify · 2024-06-24T21:15:42Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`fc1a79b`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6691b00a51a07f0008a7b77c

codecov · 2024-06-24T21:16:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.64%. Comparing base (ebf2398) to head (fc1a79b).
Report is 33 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9560   +/-   ##
=======================================
  Coverage   51.64%   51.64%           
=======================================
  Files        1255     1255           
  Lines      152618   152618           
  Branches     3092     3092           
=======================================
+ Hits        78816    78822    +6     
+ Misses      73645    73639    -6     
  Partials      157      157

Flag	Coverage Δ
backend	`43.97% <ø> (+0.01%)`	⬆️
harness	`72.80% <ø> (ø)`
web	`48.63% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes

harness/determined/experimental/core_v2/_core_v2.py

azhou-determined · 2024-07-01T20:29:36Z

i think we should probably deprecate before we remove. this would be a major breaking change for anyone using detached mode, and even though this feature is "experimental", we shouldn't break user code without some deprecation warning before.

so i'd keep the existing init() parameters, but add deprecation warnings if they're provided

harness/determined/experimental/core_v2/_core_v2.py

azhou-determined · 2024-07-02T18:03:00Z

harness/determined/experimental/core_v2/_core_v2.py

@@ -121,8 +75,7 @@ def _set_globals() -> None:

 def _init_context(
    client: experimental.Determined,
-    defaults: Optional[DefaultConfig] = None,
-    unmanaged: Optional[UnmanagedConfig] = None,
+    unmanaged_config: Optional[UnmanagedConfig] = None,


seeing as how we want to merge the configs, what if we just call it config?

azhou-determined · 2024-07-09T21:43:36Z

harness/determined/experimental/core_v2/_core_v2.py

@@ -131,6 +148,13 @@ def _init_context(
    info = determined.get_cluster_info()
    if info is not None and info.task_type == "TRIAL":
        # Managed trials.
+        if config is not None or defaults is not None or unmanaged is not None:
+            warnings.warn(
+                "Running experiment in managed mode ignores all config"


formatting/line split is weird here, line length should be 100 max

azhou-determined · 2024-07-09T21:48:48Z

harness/determined/experimental/core_v2/_core_v2.py

@@ -121,6 +137,7 @@ def _set_globals() -> None:

 def _init_context(


this is an internal/private method, so i think it makes sense to no longer pass in defaults and unmanaged here.

since we've deprecated defaults and unmanaged, can we just do the merge logic (construct the new config object) upfront (in init() and init_context())? this would reflect our intention to remove the public facing parameters, and would make the actual deprecation much easier. currently, we're saying it's deprecated but we're actually still using the objects, and whoever deprecates them later on would need to know context of detached mode to properly deprecate them.

azhou-determined · 2024-07-09T21:51:54Z

harness/determined/experimental/core_v2/_core_v2.py

+
+    if defaults is not None or unmanaged is not None:
+        warnings.warn(
+            "Determined.experimental.core_v2.init is deprecating `defaults` and `unmanaged`."


nit: wording, maybe something like

'defaults' and 'unmanaged' parameters have been deprecated and will be removed in a future version. Please use `config` instead.

to be more in line with our current deprecation messages

azhou-determined · 2024-07-09T21:54:42Z

this needs a release note

azhou-determined · 2024-07-10T15:58:10Z

harness/determined/experimental/core_v2/_core_v2.py

@@ -22,7 +23,7 @@
 @dataclasses.dataclass
 class DefaultConfig:


might make sense to rename this to just "Config" as well, since we've renamed DefaultConfig/UnmanagedConfig

I'm not sure about renaming this class, isn't that the same as removing this public class?

you can rename this one but create another class called DefaultConfig that's basically a dummy class with an __init__ that logs a deprecation warning.

Got it, thanks!

azhou-determined

all my remaining comments are nits/thoughts. feel free to merge it as is.

nice work!! 🎊 🎉

azhou-determined · 2024-07-11T18:15:16Z

e2e_tests/tests/fixtures/unmanaged/checkpoint_distributed.py

            external_experiment_id=name,
            external_trial_id=name,
        ),
+        checkpoint_storage="/tmp/determined-cp",


nit: pass this into the core_v2.Config? same for other e2e test fixtures

azhou-determined · 2024-07-11T18:17:54Z

examples/features/unmanaged/ptl/PTL-Demo.ipynb

@@ -77,7 +77,7 @@
    "from determined.experimental import core_v2\n",
    "from determined.lightning.experimental import DetLogger\n",
    "\n",
-    "det_logger = DetLogger(defaults=core_v2.DefaultConfig(name=\"ptl-demo\"))\n",
+    "det_logger = DetLogger(unmanaged_config=core_v2.UnmanagedConfig(name=\"ptl-demo\"))\n",


we should probably also update DetLogger with the new merged config. i think it's fine if it's done in a separate ticket/PR though.

azhou-determined · 2024-07-11T19:22:06Z

harness/determined/experimental/core_v2/_core_v2.py

+    DEPRECATED: Use `Config` as it contains default config.
+    """
+
+    def __init__(self, **kargs: Any) -> None:


nit: kwargs instead of kargs

azhou-determined · 2024-07-11T21:08:20Z

harness/determined/experimental/core_v2/_core_v2.py

+    defaults: Optional[DefaultConfig],
+    unmanaged: Optional[UnmanagedConfig],
+) -> Config:
+    if defaults is not None or unmanaged is not None:


nit: this method is a little convoluted with the combined if/elses around unmanaged vs which configs are passed in. partially my fault, i thought separating out the config merging logic would make it easier to read, but i see now we need all these checks in different places.

maybe if we just bubbled all these checks up to both of the callers (init and init_context) it'd be less confusing? i don't know, maybe like:

def init/init_context(...): ... # first check for deprecated fields and log warning if defaults is not None or unmanaged is not None: ... # now check for on cluster/trial and just return if we are info = determined.get_cluster_info() if info is not None and info.task_type == "TRIAL": # Managed trials. _context = core_v2._make_v2_context( ... ) return _context # ok now we know we're not on cluster, we're doing unmanaged stuff now config = config or Config( project=defaults.project or unmanaged.project, workspace=defaults.workspace or unmanaged.workspace, ... ) # now we can make the unmanaged context, this could be renamed to _init_unmanaged_context _context = _init_context( config=config or _merge_config(defaults, unmanaged), ... ) ...

yeah, we'd have a chunk of duplicated code between the two init methods, but in general i feel like validation/deprecation checks should close to the user-facing method anyway.

i dunno. i'm nitpicking here, i think it's fine as is, just something i thought about. totally up to you.

azhou-determined · 2024-07-11T21:11:51Z

harness/determined/experimental/core_v2/_core_v2.py

+        return config
+    info = determined.get_cluster_info()
+    if defaults is None and info is None:
+        raise NotImplementedError(


nit: i know this was here before, but i don't think NotImplementedError is technically/pythonically the right exception to throw here. prolly ValueError is a better fit. not important though.

gt2345 requested a review from a team as a code owner June 24, 2024 21:15

gt2345 requested a review from MikhailKardash June 24, 2024 21:15

cla-bot bot added the cla-signed label Jun 24, 2024

gt2345 requested review from azhou-determined and removed request for MikhailKardash June 24, 2024 21:15

gt2345 requested a review from ioga July 1, 2024 16:52

azhou-determined reviewed Jul 1, 2024

View reviewed changes

harness/determined/experimental/core_v2/_core_v2.py Outdated Show resolved Hide resolved

azhou-determined reviewed Jul 1, 2024

View reviewed changes

harness/determined/experimental/core_v2/_core_v2.py Outdated Show resolved Hide resolved

azhou-determined reviewed Jul 2, 2024

View reviewed changes

gt2345 added 5 commits July 3, 2024 11:36

remove DefaultConfig

ee2e2df

change examples

99d1fd8

fmt

7c18eb7

fix tests

873eccf

add deprecate message

7174d36

gt2345 force-pushed the gt/441-core_v2-init branch from 80e8f8a to 7174d36 Compare July 3, 2024 19:33

gt2345 added 2 commits July 3, 2024 14:41

remove comments

47a1bd9

add warning

fe9ac65

gt2345 requested a review from azhou-determined July 3, 2024 21:12

fmt

74b6adf

azhou-determined reviewed Jul 9, 2024

View reviewed changes

azhou-determined reviewed Jul 10, 2024

View reviewed changes

rename DefaultConfig to Config

9c36114

determined-ci requested a review from a team July 10, 2024 20:58

determined-ci added the documentation Improvements or additions to documentation label Jul 10, 2024

gt2345 added 2 commits July 10, 2024 16:06

release note

8e758c9

fmt

c1d7e61

gt2345 added 5 commits July 10, 2024 16:22

remove testing example

6502b21

typing

56de402

fix

8a0682f

release note

c7c4c15

doc fmt

15f0b48

gt2345 requested a review from azhou-determined July 11, 2024 19:17

azhou-determined approved these changes Jul 11, 2024

View reviewed changes

gt2345 added 3 commits July 12, 2024 14:56

fixes

9234bc0

fix

0468386

typo

fc1a79b

gt2345 merged commit 02da2a2 into main Jul 15, 2024
82 of 95 checks passed

gt2345 deleted the gt/441-core_v2-init branch July 15, 2024 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Improve core v2 init API [MD-441] #9560

chore: Improve core v2 init API [MD-441] #9560

gt2345 commented Jun 24, 2024 •

edited

Loading

netlify bot commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

azhou-determined commented Jul 1, 2024

azhou-determined Jul 2, 2024

azhou-determined Jul 9, 2024

azhou-determined Jul 9, 2024

azhou-determined Jul 9, 2024

azhou-determined commented Jul 9, 2024

azhou-determined Jul 10, 2024

gt2345 Jul 10, 2024

azhou-determined Jul 10, 2024

gt2345 Jul 11, 2024

azhou-determined left a comment

azhou-determined Jul 11, 2024

azhou-determined Jul 11, 2024

azhou-determined Jul 11, 2024

azhou-determined Jul 11, 2024

azhou-determined Jul 11, 2024

		@@ -121,6 +137,7 @@ def _set_globals() -> None:

		def _init_context(

chore: Improve core v2 init API [MD-441] #9560

chore: Improve core v2 init API [MD-441] #9560

Conversation

gt2345 commented Jun 24, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Jun 24, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

azhou-determined commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azhou-determined commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azhou-determined left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gt2345 commented Jun 24, 2024 •

edited

Loading

netlify bot commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading