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

Fix an error in TableArtifactViewer when keys have an empty string #913

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

nabenabe0928
Copy link
Collaborator

Contributor License Agreement

This repository (optuna-dashboard) and Goptuna share common code.
This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

This issue relates to

What does this implement/fix? Explain your changes.

In my implementation, I replaced an empty string in keys with " ", which circumvents the error.

Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

@nabenabe0928 Thank you for your pull request! The changes look good, but the cause of issue #911 appears to be that Papa.parse() does not skip empty lines by default, leading to the result shown below:

Screenshot 2024-07-31 16 43 54

Could you pass the skipEmptyLines: true option to Papa.parse()?

$ git diff
diff --git a/optuna_dashboard/ts/components/Artifact/TableArtifactViewer.tsx b/optuna_dashboard/ts/components/Artifact/TableArtifactViewer.tsx
index a9a2414..644dbff 100644
--- a/optuna_dashboard/ts/components/Artifact/TableArtifactViewer.tsx
+++ b/optuna_dashboard/ts/components/Artifact/TableArtifactViewer.tsx
@@ -143,6 +143,7 @@ const loadCSV = (props: TableArtifactViewerProps): Promise<Data[]> => {
     Papa.parse(props.src, {
       header: true,
       download: true,
+      skipEmptyLines: true,
       complete: (results: Papa.ParseResult<Data>) => {
         resolve(results?.data)
       },

https://www.papaparse.com/docs

@c-bata c-bata self-assigned this Aug 1, 2024
@nabenabe0928
Copy link
Collaborator Author

@c-bata I tried your modification, but did not work (still whites out...)

@c-bata
Copy link
Member

c-bata commented Aug 1, 2024

@nabenabe0928 Hmm... Could you please try the hard refresh? The patch seems to work fine on my environment actually.

Screenshot 2024-08-01 12 50 32

@nabenabe0928
Copy link
Collaborator Author

nabenabe0928 commented Aug 1, 2024

image

image

I tried ctrl+f5 and the source is updated as highlighted.
However, I still get the same error, but I do not know why.
It could be a linux-related issue:(

Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

Thank you for your efforts to reproduce! It appears I misunderstood the cause of issue #911. Without the changes made in this PR, I also encountered the error you ran into.

Regarding the patch I suggested, it should be applied. However, it can be addressed in a follow-up. LGTM.

@c-bata c-bata merged commit 5802970 into optuna:main Aug 1, 2024
9 checks passed
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