docs: adjust experiment pages snippets - #850
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Card links check✅ No broken Card links found. Checked external links in 1.1s |
✅ Markdown Lint PassedAll markdown files meet the linting standards! 🎉 |
|
✅ Mintlify Dev Server Started Successfully! The documentation can be served without errors. |
|
@xke please review |
- Rename dataset version variable for clarity (content_of_specific_version) - Remove unused 'Get an existing logger' section from running-experiments - Add Beta snippets for Custom dataset evaluation and Custom metrics - Replace GalileoMetrics with Metric.metrics.X across python-beta experiment snippets - Add Beta code for 'Run your app using an experiment' (rag-and-tools) - Add Beta version of OTEL run-experiment snippet - Drop misleading 'works unchanged on the Beta SDK' notes from python snippets - Guard Dataset.get() None returns in python-beta function-based experiments
85a9f85 to
8020c02
Compare
|
Optional cosmetic nits (not blockers)
|
|
@xke please review |
Add Python (Beta) snippet siblings for the observability nav group, mirroring the experiments-pages rollout (PR #850). Beta variants demonstrate the new `LogStream` class for routing logs via `LogStream.context()` instead of `galileo_context(project=..., log_stream=...)`.
|
/astra review |
|
|
||
| You will need to change your code to use this instead of creating a new logger and starting a new trace. | ||
|
|
||
| #### Get an existing logger and check for an existing trace |
There was a problem hiding this comment.
why did this section get removed? https://docs.galileo.ai/sdk-api/experiments/running-experiments#get-an-existing-logger-and-check-for-an-existing-trace
There was a problem hiding this comment.
Good catch — that removal wasn't intentional. Restored the section, its 3 CodeGroup snippets, the 6 Python/TypeScript imports, and the <Note> link to the section anchor in 0407be5.
There was a problem hiding this comment.
⚠️ This review was generated by an AI agent (Astra) and may contain mistakes. Please verify all suggestions independently.
Verdict: approve — Docs-only PR adding Python (Beta) snippets; all SDK APIs verified correct against galileo==2.2.0, no broken anchors or imports. Only minor consistency/style issues remain.
General Comments
-
🟡 minor (design): The Beta snippets aren't consistent about how they obtain an existing dataset before calling
run_experiment/Experiment(...): -
existing-dataset.mdxandexisting-dataset-log.mdx(Beta) fetch the dataset, guard againstNone, then passdataset_name=dataset.name(which makes the SDK look the dataset up a second time). -
prompt.mdx(Beta) inlinesdataset=Dataset.get(name="countries")with noNonecheck, so if the dataset doesn't exist it silently passesNoneintoExperiment(...)and the failure surfaces only at.create()with a less helpful error.
Picking one pattern across all four Beta snippets would make the examples easier to copy-paste correctly. The cleanest version, IMO, is to fetch once, guard for None, then pass dataset=dataset — both the legacy run_experiment and the new Experiment accept a Dataset object directly, so there's no need for the second name lookup.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
snippets/code/python/concepts/experiments/get-logger.mdx:1-1: After this PR, the snippet filessnippets/code/python/concepts/experiments/{get-logger,get-current-parent,logging-with-parent-check}.mdxand their TypeScript counterparts (snippets/code/typescript/concepts/experiments/{get-logger,get-current-parent,logging-with-parent-check}.mdx) are no longer imported anywhere in the docs. Worth deleting in a follow-up cleanup PR so they don't drift out of date.snippets/code/python/concepts/experiments/prompt.mdx:26-26: Pre-existing double-space typo# 1b. (OPTIONAL) If the prompt already exists, fetch it:— this PR fixed the same typo on line 13 (1a) but the matching typo on line 26 (1b) is left in place. Trivially fixable in a follow-up.
| experiment.create() | ||
| results = experiment.run() |
There was a problem hiding this comment.
🔵 nit (documentation): Per the SDK (experiment.py:1060-1101), Experiment.create() already triggers the run via trigger=True; Experiment.run() is documented as existing only for backward compatibility with the create().run() chain pattern. Calling both back-to-back here isn't wrong, but it suggests .run() is what kicks off the experiment, which is misleading for a Beta-introduction snippet. Consider either dropping the .run() line or merging the two: results = experiment.create().run(). Same comment applies to snippets/code/python-beta/concepts/experiments/generated-output-inline.mdx:44-45.
…' section Reverts the inadvertent removal of the section (and its 3 CodeGroup snippets + Python/TypeScript imports) flagged by @xke in PR review. Also restores the <Note> link to the section anchor.
|
/astra review |
There was a problem hiding this comment.
⚠️ This review was generated by an AI agent (Astra) and may contain mistakes. Please verify all suggestions independently.
Verdict: request_changes — Three of four prior review comments (Dataset.get None-guard, redundant .run() after .create(), and dataset=dataset vs dataset_name=dataset.name) remain unaddressed in the latest push, and the same Dataset.get None-return pitfall also applies to the commented Prompt.get example in prompt.mdx.
General Comments
-
🟠 major (documentation): Of the four existing review comments, only xke's question about the removed
### Get an existing logger and check for an existing tracesection has been addressed (restored in 0407be5). The three galileo-astra[bot] findings are still applicable to HEAD and I independently verified them against the installedgalileoSDK source — they are correct, not noise: -
python-beta/concepts/experiments/prompt.mdx:30—Dataset.get(name=...)returnsNonewhen the dataset isn't found (galileo/dataset.py:237-238). Inlining it insideExperiment(dataset=...)silently sets no dataset (constructor atgalileo/experiment.py:320skips whendataset is None); the failure surfaces later with a less obvious error. The peer Beta snippets in this PR (existing-dataset.mdx,existing-dataset-log.mdx) already guard withif dataset is None: raise ValueError(...). Makeprompt.mdxmatch. -
python-beta/concepts/experiments/prompt.mdx:44andpython-beta/concepts/experiments/generated-output-inline.mdx:44—Experiment.create()already triggers the run (galileo/experiment.py:476,trigger=True).Experiment.run()only exists for backward compat with thecreate().run()chain (docstring atexperiment.py:1060-1066). The snippet works, but presentingexperiment.create(); results = experiment.run()as the canonical Beta pattern is misleading for new users. Pick one: drop.run()(and assignexperiment.create()if you need the return), or use the chainresults = experiment.create().run(). -
python-beta/concepts/experiments/existing-dataset-log.mdx:18andpython-beta/concepts/experiments/existing-dataset.mdx:33— you have aDatasetobject in scope, so passingdataset_name=dataset.nametriggers a redundant name lookup inside the SDK. Usedataset=dataset.
Either apply the fixes or reply with the reasoning so the thread can be resolved.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
snippets/code/python-beta/concepts/experiments/custom-dataset.mdx:15-15: Usesmodel="gpt-4"while every other snippet in this Beta suite usesgpt-4o. The legacy Python snippet has the same gpt-4 already, so this is consistency cleanup across both — out of scope for this PR but worth a follow-up.snippets/code/python-beta/concepts/experiments/existing-dataset.mdx:7-7:client = openai.OpenAI(api_key=os.environ["OPENAI_API_KEY"])is at module top level, so importing this snippet (e.g. when copy-pasted into a notebook cell that re-runs) raises immediately ifOPENAI_API_KEYis unset. The legacy Python snippet has the same pattern, so this is a pre-existing pattern in the docs — consider moving the client into the function in a follow-up.snippets/code/python-beta/concepts/experiments/custom-dataset.mdx:26-27: The# Function-based experiments still go through run_experiment; the Experiment class doesn't support function= yet.comment is repeated verbatim across six new Beta snippets (custom-dataset, custom-metrics, existing-dataset, existing-dataset-log, otel-experiment/run-experiment, rag-and-tools/experiment-main). Consider hoisting this caveat into one of the parent.mdxpages rather than echoing it in every code snippet.snippets/code/python-beta/concepts/experiments/existing-dataset-log.mdx:9-12:def llm_call(input): ... return resultreferences an undefinedresult. Matches the legacy Python snippet (also pre-existing), but readers who copy this code verbatim will get a NameError before they ever see the Galileo behavior. Replace with a more explicit placeholder likereturn "<your-llm-output>"orraise NotImplementedError(...)in a follow-up.
| ) | ||
| prompt.create() |
There was a problem hiding this comment.
🟡 minor (documentation): Prompt.get(name=...) has the same silent-None behavior as Dataset.get: it returns None when the prompt isn't found (verified at galileo/prompt.py:413-414), it does not raise. If a reader uncomments this line they will hit the same swallowed-error class the existing bot comment is calling out on line 30. Add a similar guard in the commented snippet so the example is honest about the contract.
| ) | |
| prompt.create() | |
| # 1b. (OPTIONAL) If the prompt already exists, fetch it: | |
| # prompt = Prompt.get(name="geography-prompt") | |
| # if prompt is None: | |
| # raise ValueError("Prompt 'geography-prompt' not found") |
| @@ -127,7 +135,11 @@ This is ideal for: | |||
|
|
|||
| <Note>This flow is currently supported in the Python SDK (v1.50.1+). TypeScript support uses the same API — omit `promptTemplate` to use this flow.</Note> | |||
There was a problem hiding this comment.
🟡 minor (documentation): This Note still says the flow is "currently supported in the Python SDK (v1.50.1+)." Now that a Python (Beta) snippet sits next to the legacy Python snippet, it is unclear whether the Beta variant has the same lower bound, a different one, or no constraint. Either drop the version-specific phrasing or add a parenthetical for the Beta SDK version that introduced support.
| import SnippetExperimentsGeneratedOutputInlinePythonBeta from "/snippets/code/python-beta/concepts/experiments/generated-output-inline.mdx"; | ||
|
|
||
|
|
There was a problem hiding this comment.
🔵 nit (other): Extra blank line between the new Beta import and the closing markdownlint comment — the file uses a single blank line everywhere else.
| import SnippetExperimentsGeneratedOutputInlinePythonBeta from "/snippets/code/python-beta/concepts/experiments/generated-output-inline.mdx"; | |
| import SnippetExperimentsGeneratedOutputInlinePythonBeta from "/snippets/code/python-beta/concepts/experiments/generated-output-inline.mdx"; | |
| {/* <!-- markdownlint-enable MD044 --> */} |
…t-log.mdx Co-authored-by: galileo-astra[bot] <275138225+galileo-astra[bot]@users.noreply.github.com>
Co-authored-by: galileo-astra[bot] <275138225+galileo-astra[bot]@users.noreply.github.com>
|
No activity for 30 days — this PR will be closed in 5 days unless updated. |
2 similar comments
|
No activity for 30 days — this PR will be closed in 5 days unless updated. |
|
No activity for 30 days — this PR will be closed in 5 days unless updated. |
Describe your changes
Update missing snippents in experiments pages
Shortcut ticket
For Galileo internally raised PRs only, please update this with your shortcut ticket.
SC-63600
For external PRs, please add the issue (just put the number after the # below, and GitHub will automatically create a link):
Issue: #number
Checklist before requesting a review