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

Updated to allow for introspection on GCS buckets #997

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

Conversation

bjb19
Copy link

@bjb19 bjb19 commented Feb 16, 2024

This PR allows some introspection utils that allow introspection on Google Cloud Storage Buckets. This allows for storage of the workspace on a GCS bucket provided the user is authenticated to the correct project and auth level for reading.

@mike0sv
Copy link
Collaborator

mike0sv commented Feb 19, 2024

Hi @bjb19 ! Those files are used only for development and testing respectively. If you want to change evidently requirements you would do so in setup.py file. But you'll find that we do not specify gcsfs at all (only implicitly with fsspec extra), so you can use any version you want

@bjb19
Copy link
Author

bjb19 commented Feb 19, 2024

Hi @bjb19 ! Those files are used only for development and testing respectively. If you want to change evidently requirements you would do so in setup.py file. But you'll find that we do not specify gcsfs at all (only implicitly with fsspec extra), so you can use any version you want

Thanks for that extra context. I went ahead and fixed it on how I thought it should be fixed. Let me know if that works okay.

@mike0sv
Copy link
Collaborator

mike0sv commented Feb 19, 2024

Unfortunately this doesn't work. gcsfs should not be a required dependency for evidently since evidently can work without it fine and we do not want to force all of our users to install it. .min and .dev requirement files do not need it too since we don't have tests with gcs.
If somebody wants to use gcsfs to store workspace on gcs, they need to install gcsfs separately or use fsspec extra when installing evidently (this will however install all additional cloud fs dependencies, not only gcs)

@bjb19
Copy link
Author

bjb19 commented Feb 20, 2024

use fsspec extra when installing evidently (this will however install all additional cloud fs dependencies, not only gcs)

Can you explain more about this option please? Not sure I am following. I guess instead what I can do is one of three options to try and help this.

  1. Are there different installation profiles that we can include with pip in order to allow people to install it with evidently without needing extra requirements in user files that are not actually needed for the code but rather for evidently. I am thinking similar to airflow how you can add extra providers.

  2. Documenting somewhere in the docs. I am happy to do this if I am pointed to the right direction.

  3. Adding it anyways even though it may be unneeded in some cases. It should only add about 75kb to the package.

@bjb19
Copy link
Author

bjb19 commented Feb 20, 2024

Let me know what you think

@mike0sv
Copy link
Collaborator

mike0sv commented Feb 20, 2024

Yes, what you call installation profiles is called extras. So you can run pip install evidently[fsspec] and fsspec[full] will be added to dependency list (you can see it in setup.py in extras_require key).
However I dont think we should add other extras for specific fs implementations since there are a lot of them and adding them will require us to maintain this list.
3) is not an option, because why gcsfs should get a preference? We should install other fs as well then and it will be much more than 75kb.
2) probably is the way. I think adding something like "you'll need to install additional fsspec implementation for you storage provider" somewhere here https://docs.evidentlyai.com/user-guide/monitoring/workspace_project#remote-snapshot-storage

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.

None yet

2 participants