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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update doc requirements and guide to install pandoc #10348

Merged
merged 5 commits into from Nov 14, 2023
Merged

Update doc requirements and guide to install pandoc #10348

merged 5 commits into from Nov 14, 2023

Conversation

B-Step62
Copy link
Collaborator

@B-Step62 B-Step62 commented Nov 10, 2023 •
edited

馃洜 DevTools 馃洜

Open in GitHub Codespaces

Install mlflow from this PR

pip install git+https://github.com/mlflow/mlflow.git@refs/pull/10348/merge

Checkout with GitHub CLI

gh pr checkout 10348

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Pandoc is not required for building docs, not R docs only. This changes modify requirements and set-up scripts, as well as CONTRIBUTION.md.

The set up script is tested on both Mac and Linux (Ubuntu).

How is this PR tested?

  • Existing unit/integration tests
  • New unit/integration tests
  • Manual tests

Does this PR require documentation update?

  • No. You can skip the rest of this section.
  • Yes. I've updated:
    • Examples
    • API references
    • Instructions

Release Notes

Is this a user-facing change?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

  • area/artifacts: Artifact stores and artifact logging
  • area/build: Build and test infrastructure for MLflow
  • area/docs: MLflow documentation pages
  • area/examples: Example code
  • area/gateway: AI Gateway service, Gateway client APIs, third-party Gateway integrations
  • area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • area/models: MLmodel format, model serialization/deserialization, flavors
  • area/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templates
  • area/projects: MLproject format, project running backends
  • area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • area/server-infra: MLflow Tracking server backend
  • area/tracking: Tracking Service, tracking client APIs, autologging

Interface

  • area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
  • area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
  • area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
  • area/windows: Windows support

Language

  • language/r: R APIs and clients
  • language/java: Java APIs and clients
  • language/new: Proposals for new client languages

Integrations

  • integrations/azure: Azure and Azure ML integrations
  • integrations/sagemaker: SageMaker integrations
  • integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

  • rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
  • rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
  • rn/feature - A new user-facing feature worth mentioning in the release notes
  • rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
  • rn/documentation - A user-facing documentation change worth mentioning in the release notes

@github-actions github-actions bot added area/docs Documentation issues rn/none List under Small Changes in Changelogs. labels Nov 10, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited

Documentation preview for 1ff301e will be available here when this CircleCI job completes successfully.

More info

@B-Step62 B-Step62 requested a review from harupy November 11, 2023 01:19
harupy
harupy reviewed Nov 13, 2023
View reviewed changes
CONTRIBUTING.md Outdated
([dev-env-setup.sh](https://github.com/mlflow/mlflow/blob/master/dev/dev-env-setup.sh)),
but if you manually installing dependencies,

- Pip package: run `pip install pandoc --upgrade`
Copy link
Member

Choose a reason for hiding this comment

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

Is this really required?

Copy link
Member

@harupy harupy Nov 13, 2023 •
edited

Choose a reason for hiding this comment

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

I don't have the python pandoc but can build the docs.

Copy link
Collaborator Author

@B-Step62 B-Step62 Nov 13, 2023 •
edited

Choose a reason for hiding this comment

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

Hmm good catch, I just tried same and suceeded building now. Don't quite remember where it came from.... tho I think it's good to remove. Supposedly the library is just python binding of it's functionaliry.

harupy
harupy reviewed Nov 13, 2023
View reviewed changes
CONTRIBUTING.md Outdated
- Pip package: run `pip install pandoc --upgrade`
- Haskel project: please follow [official instruction](https://pandoc.org/installing.html).

Also check you are the version of your installation via `pandoc --version`. If using Mac
Copy link
Member

Choose a reason for hiding this comment

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

Any version requirement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2.2.1 or above, will add

harupy
harupy reviewed Nov 13, 2023
View reviewed changes
dev/dev-env-setup.sh Outdated
@@ -130,7 +130,7 @@ if [ -z "$pyenv_exist" ]; then
sudo apt-get update -y
sudo apt-get install -y make build-essential libssl-dev zlib1g-dev \
libbz2-dev libreadline-dev libsqlite3-dev wget curl llvm \
libncursesw5-dev xz-utils tk-dev libxml2-dev libxmlsec1-dev libffi-dev liblzma-dev
libncursesw5-dev xz-utils tk-dev libxml2-dev libxmlsec1-dev libffi-dev liblzma-dev pandoc \
Copy link
Member

Choose a reason for hiding this comment

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

what's the version installed by apt-get?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea it's older than 2, just found these lines: https://github.com/mlflow/mlflow/blame/master/mlflow/R/mlflow/Dockerfile.dev#L14-L18

Copy link
Member

Choose a reason for hiding this comment

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

Can we use wget and dpkg to make sure pandoc is not old?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, just pushed the new revision with updated script:)

harupy
harupy reviewed Nov 13, 2023
View reviewed changes
dev/dev-env-setup.sh Outdated
echo "Homebrew is required to install $1 on MacOS. Installing in your home directory."
bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"
fi
echo "Updating brew and installing pyenv..."
Copy link
Member

Choose a reason for hiding this comment

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

do we need installing pyenv?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

auch, will fix

harupy
harupy approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@harupy harupy left a comment

Choose a reason for hiding this comment

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

LGTM

harupy
harupy reviewed Nov 13, 2023
View reviewed changes
dev/dev-env-setup.sh
Copy link
Member

@harupy harupy Nov 13, 2023 •
edited

Choose a reason for hiding this comment

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

dev/dev-env-setup.sh installs many tools = harder to succeed. We might need to consider how to make it easier to resume.

Copy link
Member

@harupy harupy Nov 13, 2023 •
edited

Choose a reason for hiding this comment

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

For pandoc, I think it's fine to just document it on CONTRIBUTING.md.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I think one way is to split it to a few logical steps and create scripts for each. Created backlog item for this.

BenWilson2
BenWilson2 approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@BenWilson2 BenWilson2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@B-Step62 B-Step62 enabled auto-merge (squash) November 13, 2023 21:35
@B-Step62
Update doc requirements and guide to install pandoc
57e1040 
Signed-off-by: B-Step62 <yuki.watanabe@databricks.com>
@B-Step62
Remove pip dep
e7c742d 
Signed-off-by: B-Step62 <yuki.watanabe@databricks.com>
@B-Step62
Update setup script
0903c68 
Signed-off-by: B-Step62 <yuki.watanabe@databricks.com>
@B-Step62
typo / nit
42c09dd 
Signed-off-by: B-Step62 <yuki.watanabe@databricks.com>
@B-Step62
fix message
1ff301e 
Signed-off-by: B-Step62 <yuki.watanabe@databricks.com>
@B-Step62 B-Step62 merged commit 4c38b0c into mlflow:master Nov 14, 2023
34 checks passed
KonakanchiSwathi pushed a commit to KonakanchiSwathi/mlflow that referenced this pull request Nov 29, 2023
@B-Step62 @BenWilson2 @KonakanchiSwathi
Update doc requirements and guide to install pandoc (mlflow#10348)
a77466b 
Signed-off-by: B-Step62 <yuki.watanabe@databricks.com>
Signed-off-by: Yuki Watanabe <31463517+B-Step62@users.noreply.github.com>
Co-authored-by: Ben Wilson <39283302+BenWilson2@users.noreply.github.com>
Signed-off-by: swathi <konakanchi.swathi@gmail.com>
@B-Step62 B-Step62 deleted the pandoc branch January 10, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harupy harupy harupy approved these changes

@BenWilson2 BenWilson2 BenWilson2 approved these changes

Assignees
No one assigned
Labels
area/docs Documentation issues rn/none List under Small Changes in Changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@B-Step62 @harupy @BenWilson2