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

Adds feature comparison table to README #7

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Adds feature comparison table to README #7

merged 3 commits into from
Feb 27, 2023

Conversation

skrawcz
Copy link
Collaborator

@skrawcz skrawcz commented Feb 25, 2023 •
edited
Loading

So that people can get a quick sense of how Hamilton stacks up against common things its compared to.

Changes

  • Adds to README

How I tested this

  • Locally

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

what it looks like:

Screen Shot 2023-02-24 at 10 09 31 PM

@skrawcz
Adds feature comparison table to README
2a29023 
So that people can get a quick sense of how Hamilton stacks
up against common things its compared to.
elijahbenizzy
elijahbenizzy reviewed Feb 25, 2023
View reviewed changes
Copy link
Collaborator

@elijahbenizzy elijahbenizzy 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, a few minor points

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
skrawcz and others added 2 commits February 25, 2023 21:08
@skrawcz
Swaps pandas for Dask
a4e6548 
Swaps pandas for dask because I think that comparison is made more often.

Also adds a row about code base organization.
@skrawcz
Fixes whitespace
7ca6176
@skrawcz skrawcz marked this pull request as ready for review February 27, 2023 02:12
@skrawcz skrawcz merged commit 923e657 into main Feb 27, 2023
@skrawcz skrawcz deleted the add_table_to_readme branch February 27, 2023 02:15
elijahbenizzy added a commit that referenced this pull request 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
Reviewers

@elijahbenizzy elijahbenizzy elijahbenizzy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@skrawcz @elijahbenizzy