-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[FEATURE] Add requirements-dev-lite.txt and update tests/docs #4273
Changes from all commits
15ec0f7
ed0b24a
6b584b6
8dfbe84
0d56b41
08503f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -385,7 +385,7 @@ def get_dataset( | |
return PandasDataset(df, profiler=profiler, caching=caching) | ||
|
||
elif dataset_type == "sqlite": | ||
if not create_engine: | ||
if not create_engine or not SQLITE_TYPES: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It scares me that we need to make these kinds of changes. This reinforces my view that we should not be relying on imports to determine which backends to test against. Instead, I would strongly advocate for an explicit config file for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When imports fail for those dialects, we are setting the |
||
return None | ||
|
||
engine = create_engine(get_sqlite_connection_url(sqlite_db_path=sqlite_db_path)) | ||
|
@@ -445,7 +445,7 @@ def get_dataset( | |
) | ||
|
||
elif dataset_type == "postgresql": | ||
if not create_engine: | ||
if not create_engine or not POSTGRESQL_TYPES: | ||
return None | ||
|
||
# Create a new database | ||
|
@@ -508,7 +508,7 @@ def get_dataset( | |
) | ||
|
||
elif dataset_type == "mysql": | ||
if not create_engine: | ||
if not create_engine or not MYSQL_TYPES: | ||
return None | ||
|
||
db_hostname = os.getenv("GE_TEST_LOCAL_DB_HOSTNAME", "localhost") | ||
|
@@ -596,7 +596,7 @@ def get_dataset( | |
) | ||
|
||
elif dataset_type == "mssql": | ||
if not create_engine: | ||
if not create_engine or not MSSQL_TYPES: | ||
return None | ||
|
||
db_hostname = os.getenv("GE_TEST_LOCAL_DB_HOSTNAME", "localhost") | ||
|
@@ -967,20 +967,34 @@ def build_sa_validator_with_data( | |
sqlite_db_path=None, | ||
batch_definition: Optional[BatchDefinition] = None, | ||
): | ||
dialect_classes = { | ||
"sqlite": sqlitetypes.dialect, | ||
"postgresql": postgresqltypes.dialect, | ||
"mysql": mysqltypes.dialect, | ||
"mssql": mssqltypes.dialect, | ||
"bigquery": sqla_bigquery.BigQueryDialect, | ||
} | ||
dialect_types = { | ||
"sqlite": SQLITE_TYPES, | ||
"postgresql": POSTGRESQL_TYPES, | ||
"mysql": MYSQL_TYPES, | ||
"mssql": MSSQL_TYPES, | ||
"bigquery": BIGQUERY_TYPES, | ||
} | ||
dialect_classes = {} | ||
dialect_types = {} | ||
try: | ||
dialect_classes["sqlite"] = sqlitetypes.dialect | ||
dialect_types["sqlite"] = SQLITE_TYPES | ||
except AttributeError: | ||
pass | ||
try: | ||
dialect_classes["postgresql"] = postgresqltypes.dialect | ||
dialect_types["postgresql"] = POSTGRESQL_TYPES | ||
except AttributeError: | ||
pass | ||
try: | ||
dialect_classes["mysql"] = mysqltypes.dialect | ||
dialect_types["mysql"] = MYSQL_TYPES | ||
except AttributeError: | ||
pass | ||
try: | ||
dialect_classes["mssql"] = mssqltypes.dialect | ||
dialect_types["mssql"] = MSSQL_TYPES | ||
except AttributeError: | ||
pass | ||
try: | ||
dialect_classes["bigquery"] = sqla_bigquery.BigQueryDialect | ||
dialect_types["bigquery"] = BIGQUERY_TYPES | ||
except AttributeError: | ||
pass | ||
Comment on lines
+972
to
+996
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we maybe for-loop this? And perhaps add a logging statement? dialects: List[Tuple[x, y]] = [
("sqlite", sqlitetypes.dialect, SQLITE_TYPES)
...
]
for name, class_, type_ in dialects:
try:
dialect_classes[name] = class_
dialect_types[name = type_
except AttributeError:
logging.debug(...) What's the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NoneType has no attribute whatever. A lot of try/except ImportError will just end up setting whatever failed import to None. |
||
|
||
db_hostname = os.getenv("GE_TEST_LOCAL_DB_HOSTNAME", "localhost") | ||
if sa_engine_name == "sqlite": | ||
engine = create_engine(get_sqlite_connection_url(sqlite_db_path)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
|
||
--requirement requirements.txt | ||
|
||
black==22.1.0 # lint | ||
boto3>=1.9 # all_tests | ||
flake8==3.8.3 # lint | ||
flask>=1.0.0 # for s3 test only (with moto) | ||
freezegun>=0.3.15 # all_tests | ||
isort==5.4.2 # lint | ||
moto>=1.3.7,<2.0.0 # all_tests | ||
pyfakefs>=4.5.1 # all_tests | ||
pytest-benchmark>=3.4.1 # performance tests | ||
pytest>=5.3.5,<6.0.0 # all_tests | ||
requirements-parser>=0.2.0 # all_tests | ||
s3fs>=0.5.1 # all_tests | ||
snapshottest==0.6.0 # GE Cloud atomic renderer tests | ||
sqlalchemy>=1.3.18,<1.4.10 # sqlalchemy_tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick - maybe we should use a
:::warning
tag here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar with that syntax in markdown. With whatever theme we are using with docusaurus, inline comments like this have a bold yellow background that stands out pretty well.
Would it just become
::warning If you have a Silicon...
??