search

cvlab-columbia / viper

Code for the paper "ViperGPT: Visual Inference via Python Execution for Reasoning"
Other
1.63k stars 117 forks source link

Consider removing HiddenPrints? #22

Closed apoorvkh closed 1 year ago

apoorvkh commented 1 year ago

I was trying to run your code (e.g. from vision_processes import forward), but the program would crash without my knowledge.

I believe the program crashed within a HiddenPrints context manager, in this case at:

https://github.com/cvlab-columbia/viper/blob/03ba31e4ec3553dddd1c772599b96afba1ab98d6/vision_models.py#L1223-L1224

But accordingly, this never invoked HiddenPrints.__exit__

https://github.com/cvlab-columbia/viper/blob/03ba31e4ec3553dddd1c772599b96afba1ab98d6/utils.py#L194

So I never discovered the issue and consequently nothing else in my program was able to print anything. I had no idea why and this was very difficult to debug.

Would you please consider removing HiddenPrints from this codebase? I think it is better to show extra warnings than to risk such "unknown crashes". Thanks!

surisdi commented 1 year ago

Hi, can you comment out the line with warnings.catch_warnings(), HiddenPrints("XVLM"):? What error do you get?

And I agree that HiddenPrints can make things hard to debug, so I will add a flag to make it optional.

surisdi commented 1 year ago

Update: I made HiddenPrints inactive by default

apoorvkh commented 1 year ago

Great, thank you!

No worries about my specific issue. That was local and I fixed it (after commenting out HiddenPrints). Just wanted to point out my experience with HiddenPrints.