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

Upgrade IronPython to v2.7.8 removing dependency on .NET 3.0 #9180

Merged
merged 18 commits into from
Nov 7, 2018

Conversation

alfarok
Copy link
Contributor

@alfarok alfarok commented Oct 15, 2018

Purpose

QNTM-5357 : Upgrade IronPython to 2.7.8.1
QNTM-3509 : Bundle IronPython w/ Core

This PR upgrades IronPython to v.2.7.8.1 using NuGet and removes any .NET 3.0 dependencies as per the request from corporate Delivery and Wingman teams and allows Revit web installs to be fully silent . Additionally, it removes the committed IronPython binaries stored in the extern folder and the IronPython-2.7.3.msi from the extra folder and adds a NuGet reference to the IronPython Standard Library for v2.7.8.1. When Dynamo is built this package is restored and copied to the bin directory. The standard library is also now referenced by default.

# NO LONGER REQUIRED
import sys
sys.path.append(r"C:\Program Files (x86)\IronPython 2.7\Lib")

ironpythoninfo

Installer Changes

Installer PR

Revit 2020 no longer uses the Dynamo installer but it is still part of our CI/CD pipeline so changes will have to be made to the installer code to skip attempting to install Python using the previously included msi since IronPython will now be built and bundled with Core.

Testing

All tests are passing on the self-serve. I have manually verified in Sandbox and D4R in Revit 2020 that the correct version of IronPython is loaded from the /bin folder if a different version happens to be installed in the GAC (specifically version 2.7.3 which we used to install). If an exact match occurs it does not seem possible to prevent loading from the GAC but this should be a non-issue.

ironpythoninfo_testgraph

Also added a test verifying the IronPython assemblies are only getting loaded from a single location.
image

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner

FYIs

@aparajit-pratap

@alfarok alfarok changed the title Upgrade IronPython to v2.7.8.1 removing dependency on .NET 3.0 Upgrade IronPython to v2.7.8 removing dependency on .NET 3.0 Oct 15, 2018
@alfarok
Copy link
Contributor Author

alfarok commented Oct 15, 2018

All unit tests pass with the except of 4. This will be resolved when the version of IronPython is upgraded to 2.7.8 on the self-serve which the BRE team is already taking care of. I will wait for that work to be completed before merging.

Additionally on Wednesday we are going to speak with the Revit team regarding how they plan to deliver IronPython in the installation. See QNTM-3509

@@ -76,14 +76,6 @@ public object Marshal(object obj)

var targetType = obj.GetType();

// TODO: Remove this conversion after updating IronPython version in 2.1
// in which IronPython will support Int64
if (typeof (long) == targetType)
Copy link
Member

@mjkkirschner mjkkirschner Oct 16, 2018

Choose a reason for hiding this comment

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

was there any change to the output marshaller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I am aware of but @aparajit-pratap may have more knowledge here. This was the original change I believe:

https://github.com/DynamoDS/Dynamo/pull/8602/files

@alfarok
Copy link
Contributor Author

alfarok commented Oct 18, 2018

@mjkkirschner The self-serve was just updated to use Python 2.7.8 so the 4 failing Python tests are now passing

@alfarok
Copy link
Contributor Author

alfarok commented Oct 19, 2018

Note, a new self-serve was setup with an updated version of Python I am assuming for testing but the changes have not yet been done on the main Jenkins self-serve.

new:
https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service_SDKChange/
usual:
https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/

@mjkkirschner
Copy link
Member

mjkkirschner commented Oct 20, 2018

I'd like to consider not replacing these deps and removing everything from extern - just to figure out what that would take and if it should be part of this task or not.

@alfarok alfarok added the PTAL Please Take A Look 👀 label Oct 29, 2018
@alfarok
Copy link
Contributor Author

alfarok commented Oct 29, 2018

@mjkkirschner Think this is getting pretty close. I am running the the self-serve one more time and looking into seeing how much work is involved in loading the python library references by default if they are available.

@alfarok
Copy link
Contributor Author

alfarok commented Nov 5, 2018

@mjkkirschner I think this is ready for a final look when you are back. I am just going to add a few more tests but everything is currently passing on the self-serve.

@alfarok
Copy link
Contributor Author

alfarok commented Nov 5, 2018

Hey @ColinDayOrg @QilongTang, if either of you guys have a free moment could you take a look at this PR?

@jnealb
Copy link
Collaborator

jnealb commented Nov 5, 2018

@alfarok @mjkkirschner @ColinDayOrg @QilongTang Does the work-around for QNTM-3547 need to be revisited, eg removed?

@alfarok
Copy link
Contributor Author

alfarok commented Nov 5, 2018

@jnealb I believe I removed the work-around referenced in that report and no issues marshalling int64 correctly without throwing an error like shown in QNTM-3547

image

@ColinDayOrg
Copy link
Contributor

ColinDayOrg commented Nov 5, 2018

It is a little difficult to tell looking at raw project files like this, but it all seems good to me. :shipit:
Note that there is still an issue with the continuous-integration step, not sure what the issue there is.

@QilongTang
Copy link
Contributor

@alfarok Looking at the workflow for any 1.3.X or 2.0.X user, do they update to the Dynamo 2.1 version (which does not remove old IronPython) then simply not use

import sys
sys.path.append(r"C:\Program Files (x86)\IronPython 2.7\Lib")

and everything still works? Can you clarify?

@alfarok
Copy link
Contributor Author

alfarok commented Nov 6, 2018

@QilongTang the changes should not break any existing Python nodes but users could optionally remove those lines as they are no longer required. My understanding is that you can a import module as many times as you want in one Python program, no matter what module it is. Every subsequent import after the first accesses the cached module instead of re-evaluating it.

I do this by appending the new location of the standard library to the python engine directly and import sys by default. One repercussion of this is that autocomplete for sys won't show unless imported again by the user since it is excluded from the template. To prevent this we could add import sys to the template as well so all new instantiations of the node will include it.

Having any additional versions of IronPython installed should not influence the proper version from being loaded either.

@QilongTang
Copy link
Contributor

@alfarok Great. I just want to make sure.

  1. Previous version user can still open legacy dyns with Python nodes with this change as long as they still have old Python installed.
  2. We provide some way for users to adopt such version change.

If these two are met, I think this is safe change

@alfarok
Copy link
Contributor Author

alfarok commented Nov 6, 2018

@QilongTang @mjkkirschner we could make this change to the template to be more clear if want. Users will still have to include this statement if they want to get autocomplete when referencing 'sys' but it isn't required for the code to properly execute. Let me know what you guys think.

image

@ColinDayOrg
Copy link
Contributor

is there any downside to having the include sys line in the template? if not it seems pretty harmless and has a positive upside. (note that I don't know a lot about Python...)

@alfarok
Copy link
Contributor Author

alfarok commented Nov 6, 2018

@ColinDayOrg Doesn't seem like there would be any issues other than it's technically a duplicate line but like I mentioned every subsequent import after the first accesses the cached module instead of re-evaluating it. This is because previously existing python nodes in users graphs will not include any changes we make to the template so it has to be there for those scenarios.

@QilongTang
Copy link
Contributor

@alfarok I would support for updating the template

@mjkkirschner
Copy link
Member

@alfarok I think it is important this PR includes a test that verifies python nodes which imported the libraries from the old path continue to work.

@QilongTang
Copy link
Contributor

@alfarok @mjkkirschner Does any DynamoPythonTests cover running legacy graphs? I thought it should be covered already, no?

@mjkkirschner
Copy link
Member

Not sure -do those tests consume standard python libraries that must be imported?

@alfarok
Copy link
Contributor Author

alfarok commented Nov 7, 2018

@QilongTang @mjkkirschner I think some of this is covered here regarding loading the library and loading duplicate modules but I am adding a couple other verifications that I think will be useful

public void CanImportLibrary()

public void DuplicateCallsToImportShouldBeFine()

public void CanImportSystemLibraryAndGetCompletionData()

^BTW this new preview 👍

@alfarok
Copy link
Contributor Author

alfarok commented Nov 7, 2018

@andydandy74 @mostaphaRoudsari @Amoursol @dimven Hey guys, the team is hoping to upgrade IronPython to v2.7.8 for Dynamo 2.1 and it will now be bundled with Dynamo Core to prevent conflicts when referencing the GAC. Additionally we are now including the python standard library by default, which will be located in the root location where Dynamo is executing .

I don't believe these are breaking changes and legacy nodes should still function as expected, but we still have some additional QA testing to go through on our side. I just wanted to give you a heads up as python package creators. Feel free to bring up any concerns or tag others that may be interested.

@mjkkirschner
Copy link
Member

LGTM

@alfarok alfarok added LGTM Looks good to me and removed PTAL Please Take A Look 👀 labels Nov 7, 2018
@Amoursol
Copy link
Contributor

Amoursol commented Nov 7, 2018

@alfarok If you want any more testing material, feel free to use any of the content on the dynamoPython repo. Would be awesome if none of this breaks!

@alfarok alfarok merged commit 6db61fe into DynamoDS:master Nov 7, 2018
@mostaphaRoudsari
Copy link

@alfarok, Beautiful! Thank you for the heads up. Is this only going to affect Dynamo 2.0 or is it a change for all versions of Dynamo?

@alfarok
Copy link
Contributor Author

alfarok commented Nov 8, 2018

@mostaphaRoudsari This change will only be in 2.1 and forward so no changes to 1.3.X or 2.0.X. Feel free to ping me if any additional issues or questions arise. Hope all is well!

@erfajo
Copy link

erfajo commented Nov 23, 2018

I think there could be another option to control not only the IronPython problem concerning path issues but many other problems concerning locations of information needed by developers for Dynamo packages/nodes.

I have argued (at dynamobim forum) to include more information in the Inno Setup installer file... you could benefit much from using this on a larger scale.
https://forum.dynamobim.com/t/distribute-dynamo-in-an-organizational-context/28809/2

I could give a hand doing that. I have used Inno Setup from the beginning, so around 15-20 years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants