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.

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

Add ResultMixin implementations for Dask native types #20

Closed
HamiltonRepoMigrationBot opened this issue Feb 26, 2023 · 1 comment
Closed

Add ResultMixin implementations for Dask native types #20

HamiltonRepoMigrationBot opened this issue Feb 26, 2023 · 1 comment
Labels
enhancement New feature or request good first issue Good for newcomers migrated-from-old-repo Migrated from old repository

Comments

@HamiltonRepoMigrationBot
Copy link
Collaborator

Issue by skrawcz
Friday Feb 11, 2022 at 21:50 GMT
Originally opened as stitchfix/hamilton#75

Is your feature request related to a problem? Please describe.

We should implement useful implementations of:

class ResultMixin(object):
    """Base class housing the static function.

    Why a static function? That's because certain frameworks can only pickle a static function, not an entire
    object.
    """
    @staticmethod
    @abc.abstractmethod
    def build_result(**outputs: typing.Dict[str, typing.Any]) -> typing.Any:
        """This function builds the result given the computed values."""
        pass

for use with Dask. E.g. returning a Dask native array, dataframe, bag, etc. Currently the default is to return a pandas dataframe.

See the build_result function in DaskGraphAdapter for a reference point on how it could be used.

Describe the solution you'd like
These should probably be placed in the h_dask.py module for now. Otherwise open to naming.

Alternatively, we could include more options in DaskGraphAdapter. Open to thinking what way is the most user friendly solution going forward.

Additional context
The addition of these ResultMixins should enable a user who is using Dask, to not have to implement their own version,
instead they can use the ones that come with Hamilton.
@HamiltonRepoMigrationBot HamiltonRepoMigrationBot added dask enhancement New feature or request good first issue Good for newcomers labels Feb 26, 2023
@elijahbenizzy elijahbenizzy mentioned this issue Feb 26, 2023
Closed
@elijahbenizzy elijahbenizzy added the migrated-from-old-repo Migrated from old repository label Feb 26, 2023
@skrawcz
Copy link
Collaborator

skrawcz commented Mar 26, 2024

Closing for now. Since what we have seems to work and people are productive with Dask.

@skrawcz skrawcz closed this as completed Mar 26, 2024
elijahbenizzy added a commit that referenced this issue Sep 9, 2024
@elijahbenizzy
# This is a combination of 22 commits.
faa2912 
# 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
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers migrated-from-old-repo Migrated from old repository
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@skrawcz @elijahbenizzy @HamiltonRepoMigrationBot