💊 CI failures summary and remediations

As of commit 46c4279 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

@zou3519

Can you think of a better way than nested iterating over all the params if it's a dictionary?

sparse_params = []
for index, param in enumerate(params):
    if isinstance(param, dict):
        for d_index, d_param in enumerate(param.get("params", [])):
            if d_param.is_sparse:
                sparse_params.append([index, d_index])
    elif param.is_sparse:
        sparse_params.append(index)
if sparse_params:
    raise ValueError(
        f"Sparse params at indices {sparse_params}: SparseAdam requires dense parameter tensors"
    )

Would it make sense to let the optimizer parse the params and then check param groups after the super call? would still involve a nested check of is_sparse on all the parameters.

e.g.

for group in self.param_groups:
    for param in group['params']:
        if param.is_sparse:
            # do something

I think I prefer the first approach since it lets a user know where in their input the error is coming from.

@ranman

There are some pros and cons to each approach. For the first approach:

• (pro): We tell the user exactly where their sparse tensor is
• (con): more code
• (con): Not as robust. In the future, hypothetically, we could change the way inputs are passed in. If we ever did that we would have to modify this code because it assumes things about how inputs are passed in. However, there are probably a lot more things one would have to change if we changed how inputs are passed in.

For the second approach:

• (pro) more robust
• (pro) less code
• (con) Error message doesn't tell the user exactly where they have a sparse tensor, so they'll have to do some debugging.

I don't have a real preference, so let's go for the first approach. It might look weird but at least we're giving the user a nice error message :)

Codecov Report

Merging #43668 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #43668   +/-   ##
=======================================
  Coverage   69.34%   69.35%           
=======================================
  Files         378      378           
  Lines       46680    46690   +10     
=======================================
+ Hits        32369    32380   +11     
+ Misses      14311    14310    -1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdc3e23...46c4279. Read the comment docs.

@ranman merged this pull request in 24eea36.