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

Hotfix/stability #10

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Hotfix/stability #10

wants to merge 3 commits into from

Conversation

steffencruz
Copy link
Contributor

@steffencruz steffencruz commented Aug 21, 2023

  • Adds workaround for known wandb decode error by forcing JSON parsing in non-strict mode
  • Ensures expected metagraph block list is sorted before creating parquet dataframe

multigraph.py Outdated Show resolved Hide resolved

def relaxed_json_loads(s, *args, **kwargs):
kwargs.pop("strict", None) # remove the strict argument if it exists
return original_json_loads(s, strict=False, *args, **kwargs)
Copy link
Contributor

@eduardogr eduardogr Aug 22, 2023

Choose a reason for hiding this comment

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

why not call it explicitly like this in every json.loads call ?? or use this relaxed_json_loads.
Even though the approach is very nice in shape. I think it obscures what's really happening, and that's important for maintenance and readability of the code.

From my point of view, I'd say, let's make the call explicit as it is in this line, or use the function relaxed_json_loads, so we know something different is happening

What do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it's not my preferred solution at all, however the issue actual originates inside the wandb codebase - they enforce strict mode when pulling assets from their databases. We never actually call json decide ourselves..
I propose we put it either here or shamefully in a Util file for now

Copy link
Contributor Author

@steffencruz steffencruz Aug 22, 2023

Choose a reason for hiding this comment

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

So it's a way of overriding json globally.
Floppy actually came up with this hack because anyone who wants to analyze wandb data has to do something similar

@eduardogr
Copy link
Contributor

very nice catch!

LGTM after solving comments :)

Co-authored-by: Eduardo García <[email protected]>
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.

2 participants