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

[Work in progress] Refactor to only run get_all_models if necessary #828

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions spectacles/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,26 @@ def expired(self) -> bool:
return False if time.time() < self.expires_at else True


def log_backoff(details: dict) -> None:
logger.debug(
f"Backing off {details['wait']:0.1f} seconds after {details['tries']} tries. "
f"Error: {details['exception'].__class__.__name__}"
)


def backoff_with_exceptions(func: Callable[..., Any]) -> Callable[..., Any]:
@backoff.on_exception(
backoff.expo,
STATUS_EXCEPTIONS,
giveup=giveup_unless_bad_gateway,
max_tries=DEFAULT_RETRIES,
on_backoff=log_backoff,
)
@backoff.on_exception(
backoff.expo,
NETWORK_EXCEPTIONS,
max_tries=DEFAULT_NETWORK_RETRIES,
on_backoff=log_backoff,
)
async def wrapper(*args: Any, **kwargs: Any) -> Any:
if asyncio.iscoroutinefunction(func):
Expand Down
89 changes: 64 additions & 25 deletions spectacles/lookml.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,15 @@ def queried(self, value: bool) -> None:
for explore in self.explores:
explore.queried = value

def get_explore(self, name: str) -> Optional[Explore]:
return next(
def get_explore(
self, name: str, create_if_missing: bool = False
) -> Optional[Explore]:
explore = next(
(explore for explore in self.explores if explore.name == name), None
)
if not explore and create_if_missing:
explore = Explore(name=name, model_name=self.name)
self.explores.append(explore)

def get_errored_explores(self) -> Generator[Explore, None, None]:
for explore in self.explores:
Expand All @@ -307,6 +312,7 @@ class Project(LookMlObject):
def __init__(self, name: str, models: Sequence[Model]) -> None:
self.name = name
self.models = models
self._is_complete = False

def __eq__(self, other: Any) -> bool:
if not isinstance(other, Project):
Expand Down Expand Up @@ -344,6 +350,14 @@ def iter_dimensions(self, errored: bool = False) -> Iterable[Dimension]:
else:
yield dimension

@property
def is_complete_project(self) -> bool:
return self._is_complete

@is_complete_project.setter
def is_complete_project(self, value: bool) -> None:
self._is_complete = value

@property
def errored(self) -> Optional[bool]:
if self.queried:
Expand Down Expand Up @@ -373,15 +387,21 @@ def queried(self, value: bool) -> None:
for model in self.models:
model.queried = value

def get_model(self, name: str) -> Optional[Model]:
return next((model for model in self.models if model.name == name), None)

def get_explore(self, model: str, name: str) -> Optional[Explore]:
model_object = self.get_model(model)
def get_model(self, name: str, create_if_missing: bool = False) -> Optional[Model]:
models = next((model for model in self.models if model.name == name), None)
if models is None and create_if_missing:
models = Model(name=name, project_name=self.name, explores=[])
self.models.append(models)
return models

def get_explore(
self, model: str, name: str, create_if_missing: bool = False
) -> Optional[Explore]:
model_object = self.get_model(model, create_if_missing)
if not model_object:
return None
else:
return model_object.get_explore(name)
return model_object.get_explore(name, create_if_missing)

def get_results(
self,
Expand Down Expand Up @@ -507,28 +527,45 @@ async def build_project(
include_dimensions: bool = False,
ignore_hidden_fields: bool = False,
include_all_explores: bool = False,
get_full_project: bool = True,
) -> Project:
"""Creates an object (tree) representation of a LookML project."""
if filters is None:
filters = ["*/*"]
is_complete_project = False

if get_full_project:
models = []
fields = ["name", "project_name", "explores"]
for lookmlmodel in await client.get_lookml_models(fields=fields):
model = Model.from_json(lookmlmodel)
if model.project_name == name:
models.append(model)

if not models:
raise LookMlNotFound(
name="project-models-not-found",
title="No configured models found for the specified project.",
detail=(
f"Go to {client.base_url}/projects and confirm "
"a) at least one model exists for the project and "
"b) it has an active configuration."
),
)
is_complete_project = True

models = []
fields = ["name", "project_name", "explores"]
for lookmlmodel in await client.get_lookml_models(fields=fields):
model = Model.from_json(lookmlmodel)
if model.project_name == name:
models.append(model)

if not models:
raise LookMlNotFound(
name="project-models-not-found",
title="No configured models found for the specified project.",
detail=(
f"Go to {client.base_url}/projects and confirm "
"a) at least one model exists for the project and "
"b) it has an active configuration."
),
)
else:
# Create a project with only the models specified in the filters
logger.debug("Building project with only the filtered models")
models: Dict[str, Model] = {}
for filter in filters:
model, explore = filter.split("/")
if model not in models:
models[model] = Model(name=model, project_name=name, explores=[])
if explore not in models[model].explores:
models[model].explores.append(Explore(name=explore, model_name=model))
project = Project(name=name, models=list(models.values()))
models = project.models

# Prune to selected explores for non-content validators
if not include_all_explores:
Expand All @@ -555,4 +592,6 @@ async def build_project(
else:
project = Project(name, [m for m in models if len(m.explores) > 0])

# Indicates whether the project has all of the models/explores or just the selected ones
project.is_complete_project = is_complete_project
Comment on lines +595 to +596
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is notionally a world where you have manually passed in every explore within a project and this is true, even though get_full_project was passed as false. This may not matter, but wanted to flag it.

return project
40 changes: 31 additions & 9 deletions spectacles/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from spectacles.client import LOOKML_VALIDATION_TIMEOUT, LookerClient
from spectacles.exceptions import LookerApiError, SpectaclesException, SqlError
from spectacles.logger import GLOBAL_LOGGER as logger
from spectacles.lookml import CompiledSql, Explore, build_project
from spectacles.lookml import CompiledSql, Explore, build_project, Project
from spectacles.models import JsonDict, SkipReason
from spectacles.printer import print_header
from spectacles.utils import time_hash
Expand Down Expand Up @@ -355,6 +355,10 @@ async def validate_sql(
) -> JsonDict:
if filters is None:
filters = ["*/*"]
else:
# Only build the full project from the API if we're using a wildcard filter and not in incremental mode
get_full_project = any("*" in f for f in filters) or incremental

validator = SqlValidator(self.client, concurrency, runtime_threshold)
ephemeral = True if incremental else None
# Create explore-level tests for the desired ref
Expand All @@ -367,6 +371,7 @@ async def validate_sql(
filters=filters,
include_dimensions=True,
ignore_hidden_fields=ignore_hidden_fields,
get_full_project=get_full_project,
)
base_explores: Set[CompiledSql] = set()
if incremental:
Expand Down Expand Up @@ -532,8 +537,16 @@ async def validate_content(
exclude_personal: bool = False,
folders: Optional[List[str]] = None,
) -> JsonDict:
if filters is None:
filters = ["*/*"]
logger.debug(
f"Validating content. ref={ref}, filters={filters}, incremental={incremental}",
)
if filters is not None or filters == ["*/*"]:
# Only build the full project from the API if we're using a wildcard filter and not in incremental mode
get_full_project = any("*" in f for f in filters if f != "*/*") or (
not incremental
)
logger.debug(f"get_full_project = {get_full_project}")

if folders is None:
folders = []

Expand All @@ -552,13 +565,21 @@ async def validate_content(
name=self.project,
filters=filters,
include_all_explores=True,
get_full_project=get_full_project,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does get_full_project get defined if the if block on line 543 isn't entered?

)
explore_count = project.count_explores()
print_header(
f"Validating content based on {explore_count} "
f"{'explore' if explore_count == 1 else 'explores'}"
+ (" [incremental mode] " if incremental else "")
)
if not project.is_complete_project:
print_header(
f"Validating content based on all explores "
+ (" [incremental mode] " if incremental else "")
)
else:
print_header(
f"Validating content based on {explore_count} "
f"{'explore' if explore_count == 1 else 'explores'}"
+ (" [incremental mode] " if incremental else "")
)

await validator.validate(project)
results = project.get_results(validator="content", filters=filters)

Expand All @@ -574,6 +595,7 @@ async def validate_content(
name=self.project,
filters=filters,
include_all_explores=True,
get_full_project=get_full_project,
)
await validator.validate(target_project)
target_results = target_project.get_results(
Expand All @@ -586,7 +608,7 @@ async def validate_content(

@staticmethod
def _incremental_results(base: JsonDict, target: JsonDict) -> JsonDict:
"""Returns a new result with only the additional errors in `additional`."""
"""Returns a new result with only the additional errors in `target`."""
diff: JsonDict = {
"validator": "content",
# Start with models and explores we know passed for the base ref
Expand Down
17 changes: 14 additions & 3 deletions spectacles/validators/content.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,15 +133,26 @@ def _get_tile_type(content: Dict[str, Any]) -> str:
)

def _get_errors_from_result(
self, project: Project, result: Dict[str, Any], content_type: str
self,
project: Project,
result: Dict[str, Any],
content_type: str,
) -> List[ContentError]:
content_errors: List[ContentError] = []
create_if_missing = False
if not project.is_complete_project:
create_if_missing = True

for error in result["errors"]:
model_name = error["model_name"]
explore_name = error["explore_name"]
model: Optional[Model] = project.get_model(model_name)
model: Optional[Model] = project.get_model(
model_name, create_if_missing=create_if_missing
)
if model:
explore: Optional[Explore] = model.get_explore(name=explore_name)
explore: Optional[Explore] = model.get_explore(
name=explore_name, create_if_missing=create_if_missing
)
else:
explore = None
# Skip errors that are not associated with selected explores or existing models
Expand Down