I'm sorry, what?

If we can't fix this before the release ships, then I vote we remove this section and add it back later when we fix it.

This is basically a bug, and we're documenting it here rather than fixing it.

well, this is copied from pytorch-ug, where we document similar behavior. which isn't a bug, but a hackily-supported use case.

i'm pretty sure this isn't needed here though, since either we're in local mode, we init the dist backend and pass it into the ds.init(), or we're on cluster and it's None.

so i removed it, cc: @MikhailKardash

If we are documenting internal flags, that is a bug. It's also an oxymoron, since documenting it means it's not really internal, just a weirdly bad public API.

"it's a feature, not a bug!"

lol, IIRC at the time of landing pytorch trainer, we decided to do this because we wanted to document a shiny new local training capability but didn't have time to figure out a good way to do it.

you're right that it's a bad public API. i've put it on my todo list, and i'll fix it next week. it's not related to this PR or this feature, though.

The experiment config isn't safe for users to read from, because we automatically shim it from time to time.

The fact that it's part of the context is like, ancient legacy.

Instead, tell people to access info.trial.user_data for the data field or info.trial.hparams for hyperparameters.

One of the changes to DeepSpeedTrialContext was to remove det.EnvContext from it. This used to be accessible via context.env. I mentioned this as a potential breaking change for some users, who may have to move to the context.get_experiment_config() and context.get_hparams() methods.

Actually, should probably just tell users it's gone completely and they should read config from the cluster info.

This shouldn't technically be breaking because it was never documented, so it was not technically public.

But it's so old that it wasn't prefixed with _ because we didn't used to be good about doing that.

If it wasn't documented before, we don't need to document that it is gone, imo.

The reason I reference it is because our examples had code that used it (gpt_neox before the Trainer API rewrites, for example)

uh, I should probably tell you that @tara-det-ai told me we should mark it as deprecated but not delete it from the docs.

This is contrary to my preference, which is to let users look in old docs if they want to use old features.

But also it makes sense in this case because we don't have any deprecation warnings anywhere in the system for these fields; users would only know by looking at the docs.

Also this means we need to revert some of my docs deletions from the searcher-context-removal branch, which I realize now that I forgot to do 😞. It would be the removals from this file, I think.

That said, the other places where you remove things like "use min_validation_period to control validations" should be removed. But the actual reference here should not.

welp. ok then. i added them all back.

thanks, sorry.

was this a bug, or is this a breaking change to the API?

both? it was a bug because it was a breaking change. added to release notes.

This is the only reason we are breaking existing DeepSpeedTrials? Like, we're deprecating old paths, yes, but we're only actually breaking user code for this?

Can we include a @property-style getter that makes this not a breaking change?

something like:

    @property
    def train_micro_batch_size_per_gpu(self) -> int:
        warnings.warn(
            "DeepSpeedTrialContext.train_micro_batch_size_per_gpu has been deprecated in "
            "Determined 0.38.0; please use the context.get_train_micro_batch_size_per_gpu() getter "
            "instead."
            FutureWarning,
            stacklevel=2,
        )
        return self.get_train_micro_batch_size_per_gpu()

no longer breaking