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

enable connecting model node to metric node #9

Closed
HamiltonRepoMigrationBot opened this issue Feb 25, 2023 · 3 comments
Closed

enable connecting model node to metric node #9

HamiltonRepoMigrationBot opened this issue Feb 25, 2023 · 3 comments
Labels
migrated-from-old-repo Migrated from old repository sf-concern Issue concerning internal SF things.

Comments

@HamiltonRepoMigrationBot
Copy link
Collaborator

Issue by shellyjang
Wednesday Sep 01, 2021 at 00:30 GMT
Originally opened as stitchfix/hamilton#11


slack conversation

Current issue:

  1. model_p_something (“model” node) and prob_something (“metric” node) being a complement pair,
  2. create_database driver (and therefore its crawler) contain both nodes
  3. simulate driver (and therefore its crawler) only contains the “metric” node and not the “model” node.
  4. the simulate driver’s crawler can find the feature dependency of the metric node (presumably through the model coefficient configs)
  5. the crawlers (and therefore the drivers) are unable to find the complementary connection between the model and metric nodes;

item 5 specifically means that a person needs to know the complementary pairs (= domain knowledge; or hard-coded somewhere?) instead of DAG containing this info. A complementary pair is identified via @model decorator.

@model(GLM, 'model_p_demand_manual_by_formerautoship')
def prob_demand_manual_existing_former_autoship() -> pd.Series:
    pass

We would like there to be a systematic mapping between the complementary pairs.

@HamiltonRepoMigrationBot HamiltonRepoMigrationBot added the sf-concern Issue concerning internal SF things. label Feb 25, 2023
@HamiltonRepoMigrationBot
Copy link
Collaborator Author

Comment by elijahbenizzy
Saturday Oct 29, 2022 at 17:42 GMT


@kiwidamien @shellyjang thoughts on this? I know you've been mulling over approaches and we've discussed a few times, but its slipping my mind exactly where we ended up. I think tagging could be high-value here as well...

@HamiltonRepoMigrationBot
Copy link
Collaborator Author

Comment by shellyjang
Monday Oct 31, 2022 at 15:34 GMT


hi @elijahbenizzy this totally slipped out of the focus. And I can't seem to recall the options/approaches we had discussed at the time. Can you remind me (and if doc exists point me to) the tagging approach was? Thank you

@elijahbenizzy
Copy link
Collaborator

elijahbenizzy commented Jan 18, 2024

Closing this as this is no longer relevant -- it has to do with an implementation of a layer above Hamilton, which has been solved.

elijahbenizzy added a commit that referenced this issue Sep 9, 2024
# This is the 1st commit message:

Update graph_functions.py

Describes what to do in `graph_functions.py`
# This is the commit message #2:

Adds comments to lifecycle base
# This is the commit message #3:

Update h_ray.py with comments for ray tracking compatibility
# This is the commit message #4:

Replicate previous error

# This is the commit message #5:

Inline function, unsure if catching errors and exceptions to be handadled differently

# This is the commit message #6:

BaseDoRemoteExecute has the added Callable function that snadwisched lifecycle hooks

# This is the commit message #7:

method fails, says AssertionError about ray.remote decorator

# This is the commit message #8:

simple script for now to check telemetry, execution yield the ray.remote AssertionError

# This is the commit message #9:

passing pointer through and arguments to lifecycle wrapper into ray.remote

# This is the commit message #10:

post-execute hook for node not called

# This is the commit message #11:

finally executed only when exception occurs, hamilton tracker not executed

# This is the commit message #12:

atexit.register does not work, node keeps running inui

# This is the commit message #13:

added stop() method, but doesn't get called

# This is the commit message #14:

Ray telemtry works for single node, problem with connected nodes

# This is the commit message #15:

Ray telemtry works for single node, problem with connected nodes

# This is the commit message #16:

Ray telemtry works for single node, problem with connected nodes

# This is the commit message #17:

Fixes ray object dereferencing

Ray does not resolve nested arguments:
https://docs.ray.io/en/latest/ray-core/objects.html#passing-object-arguments

So one option is to make them all top level:

- one way to do that is to make the other arguments not clash with any
possible user parameters -- hence the `__` prefix. This is what I did.
- another way would be in the ray adapter, wrap the incoming function,
and explicitly do a ray.get() on any ray object references in the
kwargs arguments. i.e. keep the nested structure, but when the ray
task starts way for all inputs... not sure which is best, but this
now works correctly.

# This is the commit message #18:

ray works checkpoint, pre-commit fixed

# This is the commit message #19:

fixed graph level telemtry proposal

# This is the commit message #20:

pinned ruff

# This is the commit message #21:

Correct output, added option to start ray cluster

# This is the commit message #22:

Unit test mimicks the DoNodeExecute unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrated-from-old-repo Migrated from old repository sf-concern Issue concerning internal SF things.
Projects
None yet
Development

No branches or pull requests

2 participants