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 flagged files and ensure that flagging_mode="auto" saves output components as well #7704

Merged
merged 24 commits into from Mar 14, 2024

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Mar 14, 2024

Components with FileData values were being saved in their entirety in the CSV files. This PR makes it so that only the filepaths are saved. I considered modifying the flag() function, but that would lead to incompatibility with custom components and make it more complex for custom component developers to write their own flag() functions so instead I do some postprocessing after the .flag() function has been called.

Closes: #7701

Also fixed a long-standing issue where if flagging_mode="auto", the output was not being saved.

Closes: #3441

I set this to minor because although these are technically bugfixes, the behavior has been this way for such a long time, some users might see this as a breaking change.

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Mar 14, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/3c7625eb19fac7959b60abf8b65a1d319b7a82e8/gradio-4.21.0-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@3c7625eb19fac7959b60abf8b65a1d319b7a82e8#subdirectory=client/python"

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Mar 14, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Fix flagged files and ensure that flagging_mode="auto" saves output components as well

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@abidlabs abidlabs added the v: minor A change that requires a minor release label Mar 14, 2024
@abidlabs abidlabs marked this pull request as ready for review March 14, 2024 18:20
@abidlabs abidlabs changed the title Fix flagged files Fix flagged files and ensure that flagging_mode="auto" saves output components as well Mar 14, 2024
@abidlabs abidlabs marked this pull request as draft March 14, 2024 18:45
@@ -156,9 +155,6 @@ def postprocess(self, value: tuple[str] | str | None) -> None | str:
else:
return value.strip()

def flag(self, payload: Any, flag_dir: str | Path = "") -> str:
Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed, the base class takes care of this.

@@ -94,13 +93,6 @@ def example_payload(self) -> Any:
def example_value(self) -> Any:
return {"foo": "bar"}

def flag(
Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed anymore either, the base class takes care of this.

@@ -127,8 +127,8 @@ def image_classifier(inp):
Guides: using-flagging
"""

def __init__(self):
pass
def __init__(self, simplify_files: bool = True):
Copy link
Member Author

Choose a reason for hiding this comment

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

We add this parameter because example caching also uses this method, and there, we actually do want to keep the full file data

@abidlabs abidlabs marked this pull request as ready for review March 14, 2024 19:01
Copy link
Collaborator

@freddyaboulton freddyaboulton 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. Tested and works well @abidlabs !

@abidlabs
Copy link
Member Author

Thanks @freddyaboulton

@abidlabs abidlabs merged commit 95c6bc8 into main Mar 14, 2024
7 checks passed
@abidlabs abidlabs deleted the auto branch March 14, 2024 21:03
@pngwn pngwn mentioned this pull request Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v: minor A change that requires a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flagging does not handle files correctly When "allow_flagging" = auto, allow saving output as well
3 participants